From 2235924f37bf860535c69be90707801edc3c6fc1 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Mon, 6 Apr 2026 21:06:51 +1000 Subject: [PATCH] Harden OIDC, add SVG sanitization, improve form UX and security defaults Remove PKCE plain method support (S256 only), enforce openid scope requirement, filter to supported scopes, strip reserved claims from custom claims as defense-in-depth, sanitize SVG icons with Loofah, add global input padding, switch session cookies to SameSite=Lax, use Session.active scope, and remove unsafe-eval from CSP. Co-Authored-By: Claude Opus 4.6 (1M context) --- app/assets/tailwind/application.css | 6 +++ app/controllers/admin/users_controller.rb | 7 ++- app/controllers/concerns/authentication.rb | 6 +-- app/controllers/oidc_controller.rb | 34 ++++++++----- app/models/application.rb | 17 ++++++- app/models/oidc_authorization_code.rb | 2 +- app/models/user.rb | 4 +- app/services/oidc_jwt_service.rb | 9 ++-- .../initializers/content_security_policy.rb | 2 +- test/controllers/oidc_pkce_controller_test.rb | 51 +++++++++---------- test/models/pkce_authorization_code_test.rb | 29 +++++------ 11 files changed, 94 insertions(+), 73 deletions(-) diff --git a/app/assets/tailwind/application.css b/app/assets/tailwind/application.css index 65792d6..e3971af 100644 --- a/app/assets/tailwind/application.css +++ b/app/assets/tailwind/application.css @@ -3,6 +3,12 @@ @custom-variant dark (&:where(.dark, .dark *)); @layer base { + input:where([type="text"], [type="email"], [type="password"], [type="number"], [type="url"], [type="tel"], [type="search"]), + textarea, + select { + padding: 0.5rem 0.75rem; + } + .dark input:where([type="text"], [type="email"], [type="password"], [type="number"], [type="url"], [type="tel"], [type="search"]), .dark textarea, .dark select { diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 7c86216..694c0f0 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -122,15 +122,14 @@ module Admin end def user_params - # Base attributes that all admins can modify - base_params = params.require(:user).permit(:email_address, :username, :name, :password, :status, :totp_required, :custom_claims) + permitted = [:email_address, :username, :name, :password, :status, :totp_required, :custom_claims] # Only allow modifying admin status when editing other users (prevent self-demotion) if params[:id] != Current.session.user.id.to_s - base_params[:admin] = params[:user][:admin] if params[:user][:admin].present? + permitted << :admin end - base_params + params.require(:user).permit(*permitted) end end end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index c0ddd7a..8ce26c2 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -31,7 +31,7 @@ module Authentication end def find_session_by_cookie - Session.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id] + Session.active.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id] end def request_authentication @@ -58,8 +58,8 @@ module Authentication { value: session.id, httponly: true, - same_site: :none, # Allow cross-site cookies for OIDC testing - secure: true # Required for SameSite=None + same_site: :lax, + secure: true } else { diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index cc18974..b322d1f 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -1,4 +1,6 @@ class OidcController < ApplicationController + SUPPORTED_SCOPES = %w[openid profile email groups offline_access].freeze + # Discovery and JWKS endpoints are public # authorize is also unauthenticated to handle prompt=none and prompt=login specially allow_unauthenticated_access only: [:discovery, :jwks, :token, :revoke, :userinfo, :logout, :authorize] @@ -29,7 +31,7 @@ class OidcController < ApplicationController grant_types_supported: ["authorization_code", "refresh_token"], subject_types_supported: ["pairwise"], id_token_signing_alg_values_supported: ["RS256"], - scopes_supported: ["openid", "profile", "email", "groups", "offline_access"], + scopes_supported: SUPPORTED_SCOPES, token_endpoint_auth_methods_supported: ["client_secret_post", "client_secret_basic"], claims_supported: [ "sub", # Always included @@ -42,7 +44,7 @@ class OidcController < ApplicationController # Note: Custom claims are also supported but not listed here # ID-token-only claims (auth_time, acr, azp, at_hash, nonce) are not listed ], - code_challenge_methods_supported: ["plain", "S256"], + code_challenge_methods_supported: ["S256"], backchannel_logout_supported: true, backchannel_logout_session_supported: true, request_parameter_supported: false, @@ -67,7 +69,7 @@ class OidcController < ApplicationController scope = params[:scope] || "openid" response_type = params[:response_type] code_challenge = params[:code_challenge] - code_challenge_method = params[:code_challenge_method] || "plain" + code_challenge_method = params[:code_challenge_method] || "S256" # Validate client_id first (required before we can look up the application) # OAuth2 RFC 6749 Section 4.1.2.1: If client_id is missing/invalid, show error page (don't redirect) @@ -146,10 +148,10 @@ class OidcController < ApplicationController # Validate PKCE parameters if present (now we can safely redirect with error) if code_challenge.present? - unless %w[plain S256].include?(code_challenge_method) + unless code_challenge_method == "S256" Rails.logger.error "OAuth: Invalid code_challenge_method: #{code_challenge_method}" error_uri = "#{redirect_uri}?error=invalid_request" - error_uri += "&error_description=#{CGI.escape("Invalid code_challenge_method: must be 'plain' or 'S256'")}" + error_uri += "&error_description=#{CGI.escape("Invalid code_challenge_method: only 'S256' is supported")}" error_uri += "&state=#{CGI.escape(state)}" if state.present? redirect_to error_uri, allow_other_host: true return @@ -289,7 +291,15 @@ class OidcController < ApplicationController return end - requested_scopes = scope.split(" ") + requested_scopes = scope.split(" ") & SUPPORTED_SCOPES + scope = requested_scopes.join(" ") + + unless requested_scopes.include?("openid") + error_uri = "#{redirect_uri}?error=invalid_scope&error_description=#{CGI.escape("The 'openid' scope is required")}" + error_uri += "&state=#{CGI.escape(state)}" if state.present? + redirect_to error_uri, allow_other_host: true + return + end # Check if application is configured to skip consent # If so, automatically create consent and proceed without showing consent screen @@ -420,8 +430,7 @@ class OidcController < ApplicationController user = Current.session.user - # Record user consent - requested_scopes = oauth_params["scope"].split(" ") + requested_scopes = oauth_params["scope"].split(" ") & SUPPORTED_SCOPES parsed_claims = begin JSON.parse(oauth_params["claims_requests"]) rescue @@ -1041,16 +1050,14 @@ class OidcController < ApplicationController # Recreate code challenge based on method expected_challenge = case auth_code.code_challenge_method - when "plain" - code_verifier when "S256" Base64.urlsafe_encode64(Digest::SHA256.digest(code_verifier), padding: false) else return { valid: false, - error: "server_error", - error_description: "Unsupported code challenge method", - status: :internal_server_error + error: "invalid_request", + error_description: "Unsupported code challenge method: only 'S256' is supported", + status: :bad_request } end @@ -1156,6 +1163,7 @@ class OidcController < ApplicationController # id_token and/or userinfo keys, each mapping to claim requests def parse_claims_parameter(claims_string) return {} if claims_string.blank? + return nil if claims_string.length > 4096 parsed = JSON.parse(claims_string) return nil unless parsed.is_a?(Hash) diff --git a/app/models/application.rb b/app/models/application.rb index b7e8f93..ad2d81b 100644 --- a/app/models/application.rb +++ b/app/models/application.rb @@ -26,7 +26,7 @@ class Application < ApplicationRecord has_one_attached :icon - # Fix SVG content type after attachment + before_validation :sanitize_svg_icon, if: -> { attachment_changes["icon"].present? } after_save :fix_icon_content_type, if: -> { icon.attached? && saved_change_to_attribute?(:id) == false } has_many :application_groups, dependent: :destroy @@ -283,6 +283,21 @@ class Application < ApplicationRecord end end + def sanitize_svg_icon + return unless icon.content_type == "image/svg+xml" + + raw_svg = icon.download + doc = Loofah.xml_document(raw_svg) + doc.scrub!(SvgScrubber.new) + clean_svg = doc.to_xml + + icon.attach( + io: StringIO.new(clean_svg), + filename: icon.filename.to_s, + content_type: "image/svg+xml" + ) + end + def icon_validation return unless icon.attached? diff --git a/app/models/oidc_authorization_code.rb b/app/models/oidc_authorization_code.rb index 825cdd8..f9be0ba 100644 --- a/app/models/oidc_authorization_code.rb +++ b/app/models/oidc_authorization_code.rb @@ -9,7 +9,7 @@ class OidcAuthorizationCode < ApplicationRecord validates :code_hmac, presence: true, uniqueness: true validates :redirect_uri, presence: true - validates :code_challenge_method, inclusion: {in: %w[plain S256], allow_nil: true} + validates :code_challenge_method, inclusion: {in: %w[S256], allow_nil: true} validate :validate_code_challenge_format, if: -> { code_challenge.present? } scope :valid, -> { where(used: false).where("expires_at > ?", Time.current) } diff --git a/app/models/user.rb b/app/models/user.rb index 1966f93..0fa1cc4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -107,12 +107,12 @@ class User < ApplicationRecord save! # Save the updated array # Log successful backup code usage for security monitoring - Rails.logger.info "Backup code used successfully - User ID: #{id}, IP: #{Current.session&.client_ip}" + Rails.logger.info "Backup code used successfully - User ID: #{id}, IP: #{Current.session&.ip_address}" true else # Increment failed attempt counter and log for security monitoring increment_backup_code_failed_attempts - Rails.logger.warn "Failed backup code attempt - User ID: #{id}, IP: #{Current.session&.client_ip}" + Rails.logger.warn "Failed backup code attempt - User ID: #{id}, IP: #{Current.session&.ip_address}" false end end diff --git a/app/services/oidc_jwt_service.rb b/app/services/oidc_jwt_service.rb index 534f8f0..a6bf96b 100644 --- a/app/services/oidc_jwt_service.rb +++ b/app/services/oidc_jwt_service.rb @@ -1,6 +1,8 @@ class OidcJwtService extend ClaimsMerger + RESERVED_CLAIMS = %i[iss sub aud exp iat nbf jti nonce azp].freeze + class << self # Generate an ID token (JWT) for the user def generate_id_token(user, application, consent: nil, nonce: nil, access_token: nil, auth_time: nil, acr: nil, scopes: "openid", claims_requests: {}) @@ -79,15 +81,16 @@ class OidcJwtService # Merge custom claims from groups (arrays are combined, not overwritten) # Note: Custom claims from groups are always merged (not scope-dependent) + # Reserved claims are stripped as defense-in-depth (also validated at model layer) user.groups.each do |group| - payload = deep_merge_claims(payload, group.parsed_custom_claims) + payload = deep_merge_claims(payload, group.parsed_custom_claims.except(*RESERVED_CLAIMS)) end # Merge custom claims from user (arrays are combined, other values override) - payload = deep_merge_claims(payload, user.parsed_custom_claims) + payload = deep_merge_claims(payload, user.parsed_custom_claims.except(*RESERVED_CLAIMS)) # Merge app-specific custom claims (highest priority, arrays are combined) - payload = deep_merge_claims(payload, application.custom_claims_for_user(user)) + payload = deep_merge_claims(payload, application.custom_claims_for_user(user).except(*RESERVED_CLAIMS)) # Filter custom claims based on claims parameter # If claims parameter is present, only include requested custom claims diff --git a/config/initializers/content_security_policy.rb b/config/initializers/content_security_policy.rb index 926cfd5..607d7df 100644 --- a/config/initializers/content_security_policy.rb +++ b/config/initializers/content_security_policy.rb @@ -11,7 +11,7 @@ Rails.application.configure do # Scripts: Allow self, importmaps, unsafe-inline for Turbo/StimulusJS, and blob: for downloads # Note: unsafe_inline is needed for Stimulus controllers and Turbo navigation - policy.script_src :self, :unsafe_inline, :unsafe_eval, "blob:" + policy.script_src :self, :unsafe_inline, "blob:" # Styles: Allow self and unsafe_inline for TailwindCSS dynamic classes # and Stimulus controller style manipulations diff --git a/test/controllers/oidc_pkce_controller_test.rb b/test/controllers/oidc_pkce_controller_test.rb index 75fe1f8..238c578 100644 --- a/test/controllers/oidc_pkce_controller_test.rb +++ b/test/controllers/oidc_pkce_controller_test.rb @@ -33,8 +33,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest config = JSON.parse(@response.body) assert config.key?("code_challenge_methods_supported") - assert_includes config["code_challenge_methods_supported"], "S256" - assert_includes config["code_challenge_methods_supported"], "plain" + assert_equal ["S256"], config["code_challenge_methods_supported"] end test "authorization endpoint accepts PKCE parameters (S256)" do @@ -58,7 +57,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest assert_match(/consent/, @response.body.downcase) end - test "authorization endpoint accepts PKCE parameters (plain)" do + test "authorization endpoint rejects PKCE plain method" do code_challenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" auth_params = { @@ -74,9 +73,9 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest get "/oauth/authorize", params: auth_params - # Should show consent page (user is already authenticated) - assert_response :success - assert_match(/consent/, @response.body.downcase) + assert_response :redirect + assert_match(/error=invalid_request/, @response.location) + assert_match(/S256/, @response.location) end test "authorization endpoint rejects invalid code_challenge_method" do @@ -153,7 +152,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest assert_match(/code_verifier is required/, error["error_description"]) end - test "token endpoint requires code_verifier when PKCE was used (plain)" do + test "token endpoint requires code_verifier when PKCE was used" do # Create consent for token endpoint OidcUserConsent.create!( user: @user, @@ -163,14 +162,16 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest sid: "test-sid-123" ) - # Create authorization code with PKCE plain + code_verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + code_challenge = Base64.urlsafe_encode64(Digest::SHA256.digest(code_verifier), padding: false) + auth_code = OidcAuthorizationCode.create!( application: @application, user: @user, redirect_uri: "http://localhost:4000/callback", scope: "openid profile", - code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", - code_challenge_method: "plain", + code_challenge: code_challenge, + code_challenge_method: "S256", expires_at: 10.minutes.from_now ) @@ -274,28 +275,24 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest assert_equal "Bearer", tokens["token_type"] end - test "token endpoint accepts valid code_verifier (plain)" do - # Create consent for token endpoint - OidcUserConsent.create!( - user: @user, - application: @application, - scopes_granted: "openid profile", - granted_at: Time.current, - sid: "test-sid-123" - ) - + test "token endpoint rejects code_verifier with plain challenge method" do code_verifier = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" - # Create authorization code with PKCE plain - auth_code = OidcAuthorizationCode.create!( + # Directly insert a plain auth code to simulate legacy data + # Generate code HMAC manually since save!(validate: false) skips before_validation + plaintext_code = SecureRandom.urlsafe_base64(32) + auth_code = OidcAuthorizationCode.new( application: @application, user: @user, redirect_uri: "http://localhost:4000/callback", scope: "openid profile", - code_challenge: code_verifier, # Same as verifier for plain method + code_challenge: code_verifier, code_challenge_method: "plain", + code_hmac: OidcAuthorizationCode.compute_code_hmac(plaintext_code), expires_at: 10.minutes.from_now ) + auth_code.plaintext_code = plaintext_code + auth_code.save!(validate: false) token_params = { grant_type: "authorization_code", @@ -308,11 +305,9 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest "Authorization" => "Basic " + Base64.strict_encode64("#{@application.client_id}:#{@application.client_secret}") } - assert_response :success - tokens = JSON.parse(@response.body) - assert tokens.key?("access_token") - assert tokens.key?("id_token") - assert_equal "Bearer", tokens["token_type"] + assert_response :bad_request + body = JSON.parse(@response.body) + assert_equal "invalid_request", body["error"] end test "token endpoint works without PKCE (backward compatibility)" do diff --git a/test/models/pkce_authorization_code_test.rb b/test/models/pkce_authorization_code_test.rb index 280aa4d..241657b 100644 --- a/test/models/pkce_authorization_code_test.rb +++ b/test/models/pkce_authorization_code_test.rb @@ -38,23 +38,18 @@ class PkceAuthorizationCodeTest < ActiveSupport::TestCase assert auth_code.uses_pkce? end - test "authorization code can store PKCE challenge with plain method" do - code_challenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" - code_challenge_method = "plain" - - auth_code = OidcAuthorizationCode.create!( - application: @application, - user: @user, - redirect_uri: "http://localhost:4000/callback", - scope: "openid profile", - code_challenge: code_challenge, - code_challenge_method: code_challenge_method, - expires_at: 10.minutes.from_now - ) - - assert_equal code_challenge, auth_code.code_challenge - assert_equal code_challenge_method, auth_code.code_challenge_method - assert auth_code.uses_pkce? + test "authorization code rejects plain PKCE method" do + assert_raises(ActiveRecord::RecordInvalid) do + OidcAuthorizationCode.create!( + application: @application, + user: @user, + redirect_uri: "http://localhost:4000/callback", + scope: "openid profile", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "plain", + expires_at: 10.minutes.from_now + ) + end end test "authorization code works without PKCE (backward compatibility)" do