From cd2b4c9ba588974cb30c049319e1f77bd3fbc0b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 11:59:12 +0400 Subject: [PATCH] Address second-round review findings in the PKCE core - Enqueue the expiry job and the "app connected" notification on after_commit instead of after_create, so they only fire once the (now transactional) code exchange commits - Enforce the PKCE code_challenge format (43-128 unreserved chars, S256) with model validations, not only at the controller - Drop the now-unused RsAuthorizationCode#expired? - Cover the client_id-vs-stored-client_id check Part of #254 --- app/models/remote_storage_authorization.rb | 4 +- app/models/rs_authorization_code.rb | 8 ++-- spec/models/rs_authorization_code_spec.rb | 43 +++++++++++++------ .../rs_oauth_manager/exchange_code_spec.rb | 13 ++++++ .../rs_oauth_manager/issue_code_spec.rb | 4 +- 5 files changed, 51 insertions(+), 21 deletions(-) diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index f6a7fed..a8dfd1c 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -17,8 +17,8 @@ class RemoteStorageAuthorization < ApplicationRecord before_create :generate_token before_create :store_token_in_redis before_create :find_or_create_web_app - after_create :schedule_token_expiry - after_create :notify_user + after_commit :schedule_token_expiry, on: :create + after_commit :notify_user, on: :create before_destroy :delete_token_from_redis after_destroy :remove_token_expiry_job diff --git a/app/models/rs_authorization_code.rb b/app/models/rs_authorization_code.rb index a433ddf..482746c 100644 --- a/app/models/rs_authorization_code.rb +++ b/app/models/rs_authorization_code.rb @@ -9,6 +9,10 @@ class RsAuthorizationCode < ApplicationRecord before_validation :generate_code + validates :code_challenge, presence: true, + format: { with: /\A[A-Za-z0-9\-._~]{43,128}\z/ } + validates :code_challenge_method, inclusion: { in: %w[S256] } + scope :active, -> { where(expires_at: Time.current..) } def self.find_by_code(raw_code) @@ -17,10 +21,6 @@ class RsAuthorizationCode < ApplicationRecord active.find_by(code_digest: Digest::SHA256.hexdigest(raw_code)) end - def expired? - expires_at <= Time.current - end - private def generate_code diff --git a/spec/models/rs_authorization_code_spec.rb b/spec/models/rs_authorization_code_spec.rb index ba3dc77..4593446 100644 --- a/spec/models/rs_authorization_code_spec.rb +++ b/spec/models/rs_authorization_code_spec.rb @@ -3,17 +3,44 @@ require 'rails_helper' RSpec.describe RsAuthorizationCode, type: :model do let(:user) { create :user } - def build_code(**attrs) - RsAuthorizationCode.create!( + # Example value from RFC 7636, Appendix B + let(:challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" } + + def new_code(**attrs) + RsAuthorizationCode.new( user: user, client_id: "example.com", redirect_uri: "https://example.com", permissions: %w(documents:rw), - code_challenge: "challenge", + code_challenge: challenge, **attrs ) end + def build_code(**attrs) + new_code(**attrs).tap(&:save!) + end + + describe "validations" do + it "is valid with a well-formed S256 challenge" do + expect(new_code).to be_valid + end + + it "rejects a challenge shorter than 43 characters" do + expect(new_code(code_challenge: "short")).not_to be_valid + end + + it "rejects a challenge with invalid characters" do + expect(new_code(code_challenge: "#{'a' * 42}!")).not_to be_valid + end + + it "rejects an unsupported challenge method" do + code = new_code(code_challenge_method: "plain") + expect(code).not_to be_valid + expect(code.errors[:code_challenge_method]).to be_present + end + end + describe "#create" do it "generates a raw code and stores only its digest" do code = build_code @@ -49,14 +76,4 @@ RSpec.describe RsAuthorizationCode, type: :model do expect(RsAuthorizationCode.find_by_code(code.code)).to be_nil end end - - describe "#expired?" do - it "is false before the expiry" do - expect(build_code).not_to be_expired - end - - it "is true after the expiry" do - expect(build_code(expires_at: 1.minute.ago)).to be_expired - end - end end diff --git a/spec/services/rs_oauth_manager/exchange_code_spec.rb b/spec/services/rs_oauth_manager/exchange_code_spec.rb index 900b9e8..8bf1661 100644 --- a/spec/services/rs_oauth_manager/exchange_code_spec.rb +++ b/spec/services/rs_oauth_manager/exchange_code_spec.rb @@ -94,6 +94,19 @@ RSpec.describe RsOauthManager::ExchangeCode do .to raise_error(RsOauthManagerService::InvalidGrant) end + it "rejects a code whose stored client_id host differs from the presented one" do + code = RsAuthorizationCode.create!( + user: user, + client_id: "evil.example.org", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: code_challenge, + code_challenge_method: "S256" + ) + + expect { exchange(code) }.to raise_error(RsOauthManagerService::InvalidGrant) + end + it "rejects a bad code_verifier" do code = issue_code diff --git a/spec/services/rs_oauth_manager/issue_code_spec.rb b/spec/services/rs_oauth_manager/issue_code_spec.rb index 429c357..c80593f 100644 --- a/spec/services/rs_oauth_manager/issue_code_spec.rb +++ b/spec/services/rs_oauth_manager/issue_code_spec.rb @@ -9,7 +9,7 @@ RSpec.describe RsOauthManager::IssueCode do client_id: "example.com", redirect_uri: "https://example.com", permissions: %w(documents:rw), - code_challenge: "challenge", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "S256", expire_at: 1.month.from_now ) @@ -20,7 +20,7 @@ RSpec.describe RsOauthManager::IssueCode do expect(subject.code).to be_present expect(subject.client_id).to eq("example.com") expect(subject.permissions).to eq(%w(documents:rw)) - expect(subject.code_challenge).to eq("challenge") + expect(subject.code_challenge).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM") end it "does not create a remote storage authorization" do