dkam/clinch
1
0
Fork 0
Code Issues 7 Pull Requests Actions Packages Projects Releases Wiki Activity

Harden OIDC, add SVG sanitization, improve form UX and security defaults

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Dan Milne
2026-04-06 21:06:51 +10:00
parent c7d9df48b5
commit 2235924f37
11 changed files with 94 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
app/assets/tailwind/application.css
View File

@@ -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 {

7
app/controllers/admin/users_controller.rb
View File

@@ -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

6
app/controllers/concerns/authentication.rb
View File

@@ -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
{

34
app/controllers/oidc_controller.rb
View File

@@ -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)

17
app/models/application.rb
View File

@@ -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?

2
app/models/oidc_authorization_code.rb
View File

@@ -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) }

4
app/models/user.rb
View File

@@ -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

9
app/services/oidc_jwt_service.rb
View File

@@ -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