Bug fix for domain names with empty string instead of null. Form errors and some security fixes

VERSION

@@ -0,0 +1 @@
+2025.02

app/controllers/application_controller.rb

@@ -5,4 +5,7 @@ class ApplicationController < ActionController::Base
 
   # Changes to the importmap will invalidate the etag for HTML responses
   stale_when_importmap_changes
+
+  # CSRF protection
+  protect_from_forgery with: :exception
 end

app/controllers/oidc_controller.rb

@@ -15,11 +15,13 @@ class OidcController < ApplicationController
       jwks_uri: "#{base_url}/.well-known/jwks.json",
       end_session_endpoint: "#{base_url}/logout",
       response_types_supported: ["code"],
+      response_modes_supported: ["query"],
       subject_types_supported: ["public"],
       id_token_signing_alg_values_supported: ["RS256"],
       scopes_supported: ["openid", "profile", "email", "groups"],
       token_endpoint_auth_methods_supported: ["client_secret_post", "client_secret_basic"],
-      claims_supported: ["sub", "email", "email_verified", "name", "preferred_username", "groups", "admin"]
+      claims_supported: ["sub", "email", "email_verified", "name", "preferred_username", "groups", "admin"],
+      code_challenge_methods_supported: ["plain", "S256"]
     }
 
     render json: config
@@ -39,6 +41,8 @@ class OidcController < ApplicationController
     nonce = params[:nonce]
     scope = params[:scope] || "openid"
     response_type = params[:response_type]
+    code_challenge = params[:code_challenge]
+    code_challenge_method = params[:code_challenge_method] || "plain"
 
     # Validate required parameters
     unless client_id.present? && redirect_uri.present? && response_type == "code"
@@ -46,6 +50,20 @@ class OidcController < ApplicationController
       return
     end
 
+    # Validate PKCE parameters if present
+    if code_challenge.present?
+      unless %w[plain S256].include?(code_challenge_method)
+        render plain: "Invalid code_challenge_method. Supported: plain, S256", status: :bad_request
+        return
+      end
+
+      # Validate code challenge format (base64url-encoded, 43-128 characters)
+      unless code_challenge.match?(/\A[A-Za-z0-9\-_]{43,128}\z/)
+        render plain: "Invalid code_challenge format. Must be 43-128 characters of base64url encoding", status: :bad_request
+        return
+      end
+    end
+
     # Find the application
     @application = Application.find_by(client_id: client_id, app_type: "oidc")
     unless @application
@@ -67,7 +85,9 @@ class OidcController < ApplicationController
         redirect_uri: redirect_uri,
         state: state,
         nonce: nonce,
-        scope: scope
+        scope: scope,
+        code_challenge: code_challenge,
+        code_challenge_method: code_challenge_method
       }
       redirect_to signin_path, alert: "Please sign in to continue"
       return
@@ -96,6 +116,8 @@ class OidcController < ApplicationController
         redirect_uri: redirect_uri,
         scope: scope,
         nonce: nonce,
+        code_challenge: code_challenge,
+        code_challenge_method: code_challenge_method,
         expires_at: 10.minutes.from_now
       )
 
@@ -112,7 +134,9 @@ class OidcController < ApplicationController
       redirect_uri: redirect_uri,
       state: state,
       nonce: nonce,
-      scope: scope
+      scope: scope,
+      code_challenge: code_challenge,
+      code_challenge_method: code_challenge_method
     }
 
     # Render consent page
@@ -165,6 +189,8 @@ class OidcController < ApplicationController
       redirect_uri: oauth_params['redirect_uri'],
       scope: oauth_params['scope'],
       nonce: oauth_params['nonce'],
+      code_challenge: oauth_params['code_challenge'],
+      code_challenge_method: oauth_params['code_challenge_method'],
       expires_at: 10.minutes.from_now
     )
 
@@ -205,6 +231,7 @@ class OidcController < ApplicationController
     # Get the authorization code
     code = params[:code]
     redirect_uri = params[:redirect_uri]
+    code_verifier = params[:code_verifier]
 
     auth_code = OidcAuthorizationCode.find_by(
       application: application,
@@ -229,6 +256,16 @@ class OidcController < ApplicationController
       return
     end
 
+    # Validate PKCE if code challenge is present
+    pkce_result = validate_pkce(auth_code, code_verifier)
+    unless pkce_result[:valid]
+      render json: {
+        error: pkce_result[:error],
+        error_description: pkce_result[:error_description]
+      }, status: pkce_result[:status]
+      return
+    end
+
     # Mark code as used
     auth_code.update!(used: true)
 
@@ -342,6 +379,58 @@ class OidcController < ApplicationController
 
   private
 
+  def validate_pkce(auth_code, code_verifier)
+    # Skip PKCE validation if no code challenge was stored (legacy clients)
+    return { valid: true } unless auth_code.code_challenge.present?
+
+    # PKCE is required but no verifier provided
+    unless code_verifier.present?
+      return {
+        valid: false,
+        error: "invalid_request",
+        error_description: "code_verifier is required when code_challenge was provided",
+        status: :bad_request
+      }
+    end
+
+    # Validate code verifier format (base64url-encoded, 43-128 characters)
+    unless code_verifier.match?(/\A[A-Za-z0-9\-_]{43,128}\z/)
+      return {
+        valid: false,
+        error: "invalid_request",
+        error_description: "Invalid code_verifier format. Must be 43-128 characters of base64url encoding",
+        status: :bad_request
+      }
+    end
+
+    # 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
+                          }
+                        end
+
+    # Validate the code challenge
+    unless auth_code.code_challenge == expected_challenge
+      return {
+        valid: false,
+        error: "invalid_grant",
+        error_description: "Invalid code verifier",
+        status: :bad_request
+      }
+    end
+
+    { valid: true }
+  end
+
   def extract_client_credentials
     # Try Authorization header first (Basic auth)
     if request.headers["Authorization"]&.start_with?("Basic ")

app/helpers/application_helper.rb

@@ -19,4 +19,14 @@ module ApplicationHelper
       :smtp
     end
   end
+
+  def border_class_for(type)
+    case type.to_s
+    when 'notice' then 'border-green-200'
+    when 'alert', 'error' then 'border-red-200'
+    when 'warning' then 'border-yellow-200'
+    when 'info' then 'border-blue-200'
+    else 'border-gray-200'
+    end
+  end
 end

app/models/application.rb

@@ -18,7 +18,10 @@ class Application < ApplicationRecord
   validates :landing_url, format: { with: URI::regexp(%w[http https]), allow_nil: true, message: "must be a valid URL" }
 
   normalizes :slug, with: ->(slug) { slug.strip.downcase }
-  normalizes :domain_pattern, with: ->(pattern) { pattern&.strip&.downcase }
+  normalizes :domain_pattern, with: ->(pattern) {
+    normalized = pattern&.strip&.downcase
+    normalized.blank? ? nil : normalized
+  }
 
   before_validation :generate_client_credentials, on: :create, if: :oidc?
 

app/models/oidc_authorization_code.rb

@@ -7,6 +7,8 @@ class OidcAuthorizationCode < ApplicationRecord
 
   validates :code, presence: true, uniqueness: true
   validates :redirect_uri, presence: true
+  validates :code_challenge_method, inclusion: { in: %w[plain S256], allow_nil: true }
+  validate :validate_code_challenge_format, if: -> { code_challenge.present? }
 
   scope :valid, -> { where(used: false).where("expires_at > ?", Time.current) }
   scope :expired, -> { where("expires_at <= ?", Time.current) }
@@ -23,6 +25,10 @@ class OidcAuthorizationCode < ApplicationRecord
     update!(used: true)
   end
 
+  def uses_pkce?
+    code_challenge.present?
+  end
+
   private
 
   def generate_code
@@ -32,4 +38,11 @@ class OidcAuthorizationCode < ApplicationRecord
   def set_expiry
     self.expires_at ||= 10.minutes.from_now
   end
+
+  def validate_code_challenge_format
+    # PKCE code challenge should be base64url-encoded, 43-128 characters
+    unless code_challenge.match?(/\A[A-Za-z0-9\-_]{43,128}\z/)
+      errors.add(:code_challenge, "must be 43-128 characters of base64url encoding")
+    end
+  end
 end

app/views/shared/_flash.html.erb

@@ -71,16 +71,3 @@
     </div>
   </div>
 <% end %>
-
-<%# Helper method for border colors %>
-<%
-def border_class_for(type)
-  case type.to_s
-  when 'notice' then 'border-green-200'
-  when 'alert', 'error' then 'border-red-200'
-  when 'warning' then 'border-yellow-200'
-  when 'info' then 'border-blue-200'
-  else 'border-gray-200'
-  end
-end
-%>

app/views/shared/_form_errors.html.erb

@@ -1,5 +1,5 @@
-<%# Usage: <%= render "shared/form_errors", object: @user %> %>
-<%# Usage: <%= render "shared/form_errors", form: form %> %>
+<%# Usage: render "shared/form_errors", object: @user %>
+<%# Usage: render "shared/form_errors", form: form %>
 
 <% form_object = form.respond_to?(:object) ? form.object : (object || form) %>
 <% if form_object&.errors&.any? %>

config/environments/test.rb

@@ -53,5 +53,5 @@ Rails.application.configure do
 
   # Disable Sentry in test environment to avoid interference with tests
   # Sentry can be explicitly enabled for integration testing if needed
-  config.sentry.enabled = false
+  ENV["SENTRY_ENABLED_IN_DEVELOPMENT"] = "false"
 end

db/migrate/20251109011443_fix_empty_domain_patterns.rb

@@ -0,0 +1,17 @@
+class FixEmptyDomainPatterns < ActiveRecord::Migration[8.1]
+  def up
+    # Convert empty string domain_patterns to NULL
+    # This fixes a unique constraint issue where multiple OIDC apps
+    # had empty string domain_patterns, causing uniqueness violations
+    execute <<-SQL
+      UPDATE applications
+      SET domain_pattern = NULL
+      WHERE domain_pattern = ''
+    SQL
+  end
+
+  def down
+    # No need to reverse this - empty strings and NULL are functionally equivalent
+    # for OIDC applications where domain_pattern is not used
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.1].define(version: 2025_11_04_064114) do
+ActiveRecord::Schema[8.1].define(version: 2025_11_09_011443) do
   create_table "application_groups", force: :cascade do |t|
     t.integer "application_id", null: false
     t.datetime "created_at", null: false
@@ -69,6 +69,8 @@ ActiveRecord::Schema[8.1].define(version: 2025_11_04_064114) do
   create_table "oidc_authorization_codes", force: :cascade do |t|
     t.integer "application_id", null: false
     t.string "code", null: false
+    t.string "code_challenge"
+    t.string "code_challenge_method"
     t.datetime "created_at", null: false
     t.datetime "expires_at", null: false
     t.string "nonce"
@@ -80,6 +82,7 @@ ActiveRecord::Schema[8.1].define(version: 2025_11_04_064114) do
     t.index ["application_id", "user_id"], name: "index_oidc_authorization_codes_on_application_id_and_user_id"
     t.index ["application_id"], name: "index_oidc_authorization_codes_on_application_id"
     t.index ["code"], name: "index_oidc_authorization_codes_on_code", unique: true
+    t.index ["code_challenge"], name: "index_oidc_authorization_codes_on_code_challenge"
     t.index ["expires_at"], name: "index_oidc_authorization_codes_on_expires_at"
     t.index ["user_id"], name: "index_oidc_authorization_codes_on_user_id"
   end