Untrack SECURITY_REVIEW_TODO.md and gitignore it

Keep the findings tracker local-only; it should not be published.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

.github/workflows/build.yml

@@ -0,0 +1,133 @@
+name: Build and publish image
+
+# Publishes the multi-arch image (amd64 + arm64) to GitHub Packages
+# (ghcr.io/dkam/clinch) whenever config/initializers/version.rb changes on
+# main — a version bump IS the release. Each arch builds natively (no QEMU); a
+# merge job stitches them into one manifest tagged :vX.Y.Z (+ :latest for
+# non-pre-releases).
+#
+# To cut a release: edit Clinch::VERSION in config/initializers/version.rb,
+# commit, push. For a dev build: set a pre-release version (e.g. "1.1.0-dev") —
+# it publishes :v1.1.0-dev but does not move :latest. Or run this workflow
+# manually from the Actions tab.
+
+on:
+  push:
+    branches: [ main ]
+    paths:
+      - config/initializers/version.rb
+  workflow_dispatch:
+
+env:
+  IMAGE: ghcr.io/${{ github.repository }}
+
+jobs:
+  # Read the SemVer constant; decide whether this release moves :latest.
+  prepare:
+    runs-on: ubuntu-latest
+    outputs:
+      version: ${{ steps.version.outputs.version }}
+      latest: ${{ steps.version.outputs.latest }}
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v5
+
+      - name: Read version from config/initializers/version.rb
+        id: version
+        run: |
+          V=$(ruby -e "require './config/initializers/version'; puts Clinch::VERSION")
+          echo "version=$V" >> "$GITHUB_OUTPUT"
+          # A pre-release (e.g. 1.1.0-dev) publishes its own tag but not :latest.
+          if [[ "$V" == *-* ]]; then latest=false; else latest=true; fi
+          echo "latest=$latest" >> "$GITHUB_OUTPUT"
+          echo "Building v$V (move :latest = $latest)"
+
+  build:
+    needs: prepare
+    runs-on: ${{ matrix.runner }}
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - platform: linux/amd64
+            arch: amd64
+            runner: ubuntu-latest
+          - platform: linux/arm64
+            arch: arm64
+            runner: ubuntu-24.04-arm
+    permissions:
+      contents: read
+      packages: write
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v5
+
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+
+      - name: Log in to GitHub Container Registry
+        uses: docker/login-action@v3
+        with:
+          registry: ghcr.io
+          username: ${{ github.actor }}
+          password: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Build and push by digest
+        id: build
+        uses: docker/build-push-action@v6
+        with:
+          context: .
+          platforms: ${{ matrix.platform }}
+          cache-from: type=gha,scope=${{ matrix.arch }}
+          cache-to: type=gha,mode=max,scope=${{ matrix.arch }}
+          outputs: type=image,name=${{ env.IMAGE }},push-by-digest=true,name-canonical=true,push=true
+
+      - name: Export digest
+        run: |
+          mkdir -p /tmp/digests
+          digest="${{ steps.build.outputs.digest }}"
+          touch "/tmp/digests/${digest#sha256:}"
+
+      - name: Upload digest
+        uses: actions/upload-artifact@v4
+        with:
+          name: digests-${{ matrix.arch }}
+          path: /tmp/digests/*
+          if-no-files-found: error
+          retention-days: 1
+
+  merge:
+    needs: [prepare, build]
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+      packages: write
+    steps:
+      - name: Download digests
+        uses: actions/download-artifact@v4
+        with:
+          path: /tmp/digests
+          pattern: digests-*
+          merge-multiple: true
+
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+
+      - name: Log in to GitHub Container Registry
+        uses: docker/login-action@v3
+        with:
+          registry: ghcr.io
+          username: ${{ github.actor }}
+          password: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Create and push the multi-arch manifest
+        working-directory: /tmp/digests
+        run: |
+          tags="-t ${{ env.IMAGE }}:v${{ needs.prepare.outputs.version }}"
+          if [ "${{ needs.prepare.outputs.latest }}" = "true" ]; then
+            tags="$tags -t ${{ env.IMAGE }}:latest"
+          fi
+          docker buildx imagetools create $tags $(printf '${{ env.IMAGE }}@sha256:%s ' *)
+
+      - name: Inspect result
+        run: docker buildx imagetools inspect ${{ env.IMAGE }}:latest

.gitignore

@@ -70,3 +70,6 @@ yarn-debug.log*
 
 # Ignore bootsnap cache
 /tmp/cache/bootsnap*
+
+# Local-only: do not publish the security findings tracker
+SECURITY_REVIEW_TODO.md

SECURITY_REVIEW_TODO.md

@@ -1,110 +0,0 @@
-# Security Review — Tracking
-
-Status of findings from the multi-surface security review (OIDC/OAuth2, ForwardAuth,
-WebAuthn/TOTP, sessions, admin/config). Work landed on branch
-`security/forward-auth-and-consent-csrf`.
-
-## ✅ Done (branch `security/forward-auth-and-consent-csrf`)
-
-All HIGH findings are closed. Each fix has tests; suite is green.
-
-| Commit | Fix | Sev |
-|--------|-----|-----|
-| `703d24e` | ForwardAuth fail-open when no host header; consent endpoint CSRF | HIGH ×2 |
-| `8a095e4` | Bearer API-key skipped group check at use-time | HIGH |
-| `96a657e` | Open redirect via unvalidated `X-Forwarded-Host` in login redirect | HIGH |
-| `84ed462` | `CLINCH_HOST` made mandatory in deployed envs; dropped request-host fallback | MEDIUM |
-| `f38ac2e` | TOTP code replay within drift window (+ latent plaintext backup-code bug) | HIGH |
-| `406a79d` | SSRF via `backchannel_logout_uri` (metadata/loopback/RFC1918) | HIGH |
-| `57d7d1f` | Host-auth regex unanchored (`evil-example.com` matched) | HIGH |
-| `89bd5f1` | Disabled user could complete 2FA mid-flow / keep session; enforce active status | HIGH |
-| `cd862c7` | TOTP/backup/OAuth/PKCE `code` params not filtered from logs | MEDIUM |
-| `2426687` | `revoke_family!` didn't revoke access tokens on refresh-token reuse | HIGH |
-| `44892e3` | WebAuthn clone detection logged but didn't block; false-positive on synced passkeys | HIGH |
-| `d49e7ce` | CSP `unsafe-inline` removed (script-src + style-src → nonces) | HIGH |
-
-**Verified false positive (no change):** PKCE *is* required by default —
-`require_pkce` column defaults to `true` (`db/schema.rb`), token endpoint enforces
-it, admin UI exposes the opt-out. Operational check: confirm no legacy confidential
-apps sit on `require_pkce = false`.
-
-**Follow-up before relying on CSP change:** do one manual browser pass (DevTools
-console) on `/signin`, OAuth consent, a Turbo navigation, dark-mode toggle, and a
-WebAuthn sign-in — expect zero CSP violations. Dev is report-only so violations
-surface as warnings without breaking. Fallback if style-src surprises: keep
-`style-src 'unsafe-inline'`, ship script-src only.
-
-## ☐ Remaining — MEDIUM
-
-- [ ] **`id_token_hint` ignored at OIDC logout** — any client can redirect logout to
-      any other registered client's post-logout URI. Validate the hint's `aud` and
-      scope the redirect to that app. `app/controllers/oidc_controller.rb` (logout).
-- [ ] **`offline_access` doesn't gate refresh-token issuance** — refresh tokens are
-      minted unconditionally; gate on the granted scope.
-      `app/controllers/oidc_controller.rb` (authorization_code grant, ~line 564).
-- [ ] **CSP-report endpoint hardening** — unauthenticated, no rate limit / body-size
-      cap, logs raw CRLF (log injection). Sanitize values, cap size, rate-limit.
-      `app/controllers/api/csp_controller.rb`.
-- [ ] **Port not stripped from `X-Forwarded-Host`** in main verify + bearer paths →
-      403 outages on non-standard ports (also a correctness bug). Reuse the
-      port-stripping done in `check_forward_auth_token`.
-      `app/controllers/api/forward_auth_controller.rb`.
-- [ ] **WebAuthn `acr:"2"` without enforced user verification** — `user_verification:
-      "preferred"` lets a PIN-less key authenticate yet reports verified 2FA. Use
-      `"required"`, or downgrade `acr` to `"1"` when the UV flag is absent.
-      `app/controllers/sessions_controller.rb` (webauthn_challenge/verify),
-      `app/controllers/webauthn_controller.rb`.
-- [ ] **`RESERVED_CLAIMS` incomplete** — missing `at_hash`/`auth_time`/`acr`; and
-      `ApplicationUserClaims` has no reserved-name validation (User/Group do). Could
-      let a custom claim overwrite a security claim. `app/services/oidc_jwt_service.rb`,
-      `app/models/application_user_claim.rb`.
-- [ ] **`reset_session` not called on login** — defensive best practice for an IdP;
-      clears pre-auth session state. `app/controllers/concerns/authentication.rb`
-      (`start_new_session_for`).
-- [x] **Hardcoded private IP `192.168.2.246`** in `config/environments/production.rb`
-      — removed; it was redundant with the `192.168.0.0/16` regex already in the
-      `CLINCH_ALLOW_INTERNAL_IPS` block.
-- [ ] **CSP `form-action` widened by unvalidated `redirect_uri`** before auth — only
-      add to `form-action` if the client_id+redirect_uri is a registered pair.
-      `app/controllers/concerns/authentication.rb` (`allow_oauth_redirect_in_csp`).
-- [ ] **SVG `style` attribute permits `url()`/`expression()`** — mitigated today by
-      `Content-Disposition: attachment`, but fragile. Sanitize CSS values or drop
-      `style` from the allowlist. `app/models/svg_scrubber.rb`.
-- [ ] **WebAuthn error messages leak internals** — return generic errors to client,
-      log detail server-side. `app/controllers/sessions_controller.rb`,
-      `app/controllers/webauthn_controller.rb`.
-- [ ] **Account enumeration via webauthn challenge** — distinguishes "user not found"
-      vs "no passkey". Return a uniform message. `app/controllers/sessions_controller.rb`
-      (`webauthn_challenge`).
-- [ ] **`token_family_id` only 31 bits** (`SecureRandom.random_number(2**31)`) —
-      birthday collision ~46k; use a UUID/string. `app/models/oidc_refresh_token.rb`.
-- [ ] **Session cookie uses sequential integer DB id** — HMAC-signed so not forgeable,
-      but consider a random `token` column (Rails 8 generator default).
-      `app/models/session.rb`, `app/controllers/concerns/authentication.rb`.
-- [ ] **Login rate-limit is IP-only** — no account lockout (distributed brute force /
-      credential stuffing). Add failed-count + `locked_until` on users.
-- [ ] **Backup-code rate limit not reset on success** and is cache-based (resets on
-      cache flush). Reset on success; consider DB-backed counter. `app/models/user.rb`.
-
-## ☐ Remaining — LOW / INFO
-
-- [ ] Public clients can't revoke their own tokens (revoke endpoint requires secret).
-- [ ] Basic-auth client creds not URL-decoded per RFC 6749 §2.3.1.
-- [ ] `token_hmac` columns nullable at DB level despite model `presence: true`.
-- [ ] Group names allow commas → injection into `X-Remote-Groups` (false memberships
-      downstream). Add a format validator. `app/models/group.rb`.
-- [ ] `fa_token` leaks in redirect URL / Referer / history (60s TTL, host-bound).
-- [ ] Admin `domain_pattern` allows ReDoS — add a format validator.
-      `app/models/application.rb`.
-- [ ] Forced-TOTP-setup login path can redirect-loop (`totp_required` + no TOTP).
-- [ ] `complete_setup` creates an unprompted session for any authenticated user.
-- [ ] Password min length only 8 — consider 12 + a max (bcrypt 72-byte truncation).
-- [ ] `support_unencrypted_data: true` left enabled (TOTP secret encryption migration).
-      `config/initializers/active_record_encryption.rb`.
-- [ ] All crypto keys derived from a single `SECRET_KEY_BASE` root — document setting
-      independent `ACTIVE_RECORD_ENCRYPTION_*` keys in production.
-- [ ] Log injection via user `email_address` in ForwardAuth logs (strip CRLF / use
-      structured logging). `app/controllers/api/forward_auth_controller.rb`.
-- [ ] WebAuthn RP ID is the registrable domain (cross-subdomain credential roaming) —
-      set `CLINCH_RP_ID` to the exact host unless roaming is intended.
-      `config/initializers/webauthn.rb`.

app/controllers/admin/access_checks_controller.rb

@@ -2,17 +2,12 @@ module Admin
   class AccessChecksController < BaseController
     def new
       load_options
-    end
-
-    def create
-      load_options
       @user = User.find_by(id: params[:user_id])
       @application = Application.find_by(id: params[:application_id])
-      return render :new unless @user && @application
+      return unless @user && @application
 
       @allowed = @application.user_allowed?(@user)
       @via = @user.groups & @application.allowed_groups
-      render :new
     end
 
     private

app/views/admin/access_checks/new.html.erb

@@ -5,7 +5,7 @@
 
 <div class="bg-white dark:bg-gray-800 shadow sm:rounded-lg">
   <div class="px-4 py-5 sm:p-6">
-    <%= form_with url: admin_access_path, method: :post, class: "space-y-4" do |form| %>
+    <%= form_with url: admin_access_path, method: :get, class: "space-y-4" do |form| %>
       <div class="grid grid-cols-1 gap-4 sm:grid-cols-2">
         <div>
           <%= form.label :user_id, "User", class: "block text-sm font-medium text-gray-700 dark:text-gray-300" %>

config/environments/production.rb

@@ -157,17 +157,5 @@ Rails.application.configure do
   # Skip DNS rebinding protection for the default health check endpoint.
   config.host_authorization = {exclude: ->(request) { request.path == "/up" }}
 
-  # Sentry configuration for production
-  # Only enabled if SENTRY_DSN environment variable is set
-  if ENV["SENTRY_DSN"].present?
-    config.sentry.enabled = true
-
-    # Performance monitoring: sample 20% of transactions for traces
-    # Adjust based on your traffic volume and Sentry plan limits
-    config.sentry.traces_sample_rate = ENV.fetch("SENTRY_TRACES_SAMPLE_RATE", 0.2).to_f
-
-    # Continuous profiling: disabled by default in production due to cost
-    # Enable temporarily for performance investigations if needed
-    config.sentry.profiles_sample_rate = ENV.fetch("SENTRY_PROFILES_SAMPLE_RATE", 0.0).to_f
-  end
+  # Sentry is configured in config/initializers/sentry.rb, gated on SENTRY_DSN.
 end

config/initializers/content_security_policy.rb

@@ -53,9 +53,10 @@ Rails.application.configure do
     # Child sources: Allow self for any future iframes
     policy.child_src :self
 
-    # Additional security headers for WebAuthn
-    # Required for WebAuthn to work properly
-    policy.require_trusted_types_for :none
+    # Do not enforce Trusted Types. The only valid value for
+    # require-trusted-types-for is 'script'; there is no 'none' token, so
+    # emitting it produces an invalid directive that browsers reject. To leave
+    # Trusted Types unenforced (needed for WebAuthn), omit the directive entirely.
 
     # CSP reporting using report_uri (supported method)
     policy.report_uri "/api/csp-violation-report"

config/initializers/sentry.rb

@@ -1,62 +1,44 @@
-# Sentry configuration for error tracking and performance monitoring
-# Only initializes if SENTRY_DSN environment variable is set
+# Sentry configuration for error tracking and performance monitoring.
+# Only initializes if the SENTRY_DSN environment variable is set.
 
 return unless ENV["SENTRY_DSN"].present?
 
-Rails.application.configure do
-  config.sentry.dsn = ENV["SENTRY_DSN"]
+Sentry.init do |config|
+  config.dsn = ENV["SENTRY_DSN"]
 
-  # Set environment (defaults to Rails.env)
-  config.sentry.environment = ENV["SENTRY_ENVIRONMENT"] || Rails.env
+  # Environment label (defaults to Rails.env)
+  config.environment = ENV["SENTRY_ENVIRONMENT"] || Rails.env
 
-  # Set release version from Git or environment variable
-  config.sentry.release = ENV["SENTRY_RELEASE"] || `git rev-parse HEAD 2>/dev/null`.strip.presence || nil
+  # Release version from an env var or the current Git SHA
+  config.release = ENV["SENTRY_RELEASE"] || `git rev-parse HEAD 2>/dev/null`.strip.presence
 
-  # Sample rate for performance monitoring (0.0 to 1.0)
-  config.sentry.traces_sample_rate = ENV.fetch("SENTRY_TRACES_SAMPLE_RATE", 0.1).to_f
+  # Only report from production unless explicitly enabled elsewhere.
+  config.enabled_environments =
+    if ENV["SENTRY_ENABLED_IN_DEVELOPMENT"] == "true"
+      %w[production development]
+    else
+      %w[production]
+    end
 
-  # Enable profiling in development/staging, disable in production unless explicitly enabled
-  config.sentry.profiles_sample_rate = if Rails.env.production?
+  # Don't send cookies, request bodies, or user IPs by default.
+  config.send_default_pii = false
+
+  # Breadcrumbs for debugging
+  config.breadcrumbs_logger = [:active_support_logger, :http_logger]
+
+  # Performance monitoring sample rate (0.0 to 1.0)
+  config.traces_sample_rate = ENV.fetch("SENTRY_TRACES_SAMPLE_RATE", 0.1).to_f
+
+  # Profiling: disabled in production by default due to cost.
+  config.profiles_sample_rate =
+    if Rails.env.production?
       ENV.fetch("SENTRY_PROFILES_SAMPLE_RATE", 0.0).to_f
     else
       ENV.fetch("SENTRY_PROFILES_SAMPLE_RATE", 0.5).to_f
     end
 
-  # Include additional context
-  config.sentry.before_send = lambda do |event, hint|
-    # Filter out sensitive information
-    if event.context[:extra]
-      event.context[:extra].reject! { |key, value|
-        key.to_s.match?(/password|secret|token|key/i) || value.to_s.match?(/password|secret/i)
-      }
-    end
-
-    # Filter sensitive parameters
-    if event.context[:request]
-      event.context[:request].reject! { |key, value|
-        key.to_s.match?(/password|secret|token|key|authorization/i)
-      }
-    end
-
-    event
-  end
-
-  # Include breadcrumbs for debugging
-  config.sentry.breadcrumbs_logger = [:active_support_logger, :http_logger]
-
-  # Send session data for user context
-  config.sentry.user_context = lambda do
-    if Current.user.present?
-      {
-        id: Current.user.id,
-        email: Current.user.email_address,
-        admin: Current.user.admin?
-      }
-    end
-  end
-
   # Ignore common non-critical exceptions
-  config.sentry.excluded_exceptions += [
+  config.excluded_exceptions += [
     "ActionController::RoutingError",
     "ActionController::InvalidAuthenticityToken",
     "ActionController::UnknownFormat",
@@ -66,75 +48,38 @@ Rails.application.configure do
     "ActiveRecord::RecordNotFound"
   ]
 
-  # Add CSP-specific tags for security events
-  config.sentry.tags = lambda do
-    {
-      # Add application context
+  # Attach application/user context and scrub anything sensitive before sending.
+  config.before_send = lambda do |event, _hint|
+    event.tags = (event.tags || {}).merge(
       app_name: "clinch",
-      app_environment: Rails.env,
-      # Add CSP policy status
-      csp_enabled: defined?(Rails.application.config.content_security_policy) &&
-        Rails.application.config.content_security_policy.present?
-    }
+      app_environment: Rails.env
+    )
+
+    if defined?(Current) && Current.respond_to?(:user) && Current.user
+      event.user = (event.user || {}).merge(
+        id: Current.user.id,
+        email: Current.user.email_address,
+        admin: Current.user.admin?
+      )
     end
 
-  # Enhance before_send to handle CSP events properly
-  config.sentry.before_send = lambda do |event, hint|
-    # Filter out sensitive information
-    if event.context[:extra]
-      event.context[:extra].reject! { |key, value|
+    if event.extra.is_a?(Hash)
+      event.extra.reject! do |key, value|
         key.to_s.match?(/password|secret|token|key/i) || value.to_s.match?(/password|secret/i)
-      }
       end
-
-    # Filter sensitive parameters
-    if event.context[:request]
-      event.context[:request].reject! { |key, value|
-        key.to_s.match?(/password|secret|token|key|authorization/i)
-      }
-    end
-
-    # Special handling for CSP violations
-    if event.tags&.dig(:csp_violation)
-      # Ensure CSP violations have proper security context
-      event.context[:server] = event.context[:server] || {}
-      event.context[:server][:name] = "clinch-auth-service"
-      event.context[:server][:environment] = Rails.env
-
-      # Add additional security context
-      event.context[:extra] ||= {}
-      event.context[:extra][:security_context] = {
-        csp_reporting: true,
-        user_authenticated: event.context[:user].present?,
-        request_origin: event.context[:request]&.dig(:headers, "Origin"),
-        request_referer: event.context[:request]&.dig(:headers, "Referer")
-      }
     end
 
     event
   end
 
-  # Add CSP-specific breadcrumbs for security events
-  config.sentry.before_breadcrumb = lambda do |breadcrumb, hint|
-    # Filter out sensitive breadcrumb data
-    if breadcrumb[:data]
-      breadcrumb[:data].reject! { |key, value|
-        key.to_s.match?(/password|secret|token|key|authorization/i) ||
-          value.to_s.match?(/password|secret/i)
-      }
+  # Scrub sensitive data out of breadcrumbs.
+  config.before_breadcrumb = lambda do |breadcrumb, _hint|
+    if breadcrumb.data.is_a?(Hash)
+      breadcrumb.data.reject! do |key, value|
+        key.to_s.match?(/password|secret|token|key|authorization/i) || value.to_s.match?(/password|secret/i)
       end
-
-    # Mark CSP-related events
-    if breadcrumb[:message]&.include?("CSP Violation") ||
-        breadcrumb[:category]&.include?("csp")
-      breadcrumb[:data] ||= {}
-      breadcrumb[:data][:security_event] = true
-      breadcrumb[:data][:csp_violation] = true
     end
 
     breadcrumb
   end
-
-  # Only send errors in production unless explicitly enabled
-  config.sentry.enabled = Rails.env.production? || ENV["SENTRY_ENABLED_IN_DEVELOPMENT"] == "true"
 end

config/initializers/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module Clinch
-  VERSION = "0.16.0"
+  VERSION = "0.16.3"
 end

config/routes.rb

@@ -96,7 +96,6 @@ Rails.application.routes.draw do
     end
     resources :groups
     get "access", to: "access_checks#new"
-    post "access", to: "access_checks#create"
   end
 
   # Render dynamic PWA files from app/views/pwa/* (remember to link manifest in application.html.erb)

test/controllers/admin/access_checks_controller_test.rb

@@ -15,8 +15,8 @@ module Admin
       assert_match "alice@example.com", response.body
     end
 
-    test "create returns 'can access' with via group when user is in an allowed group" do
-      post admin_access_path, params: {
+    test "returns 'can access' with via group when user is in an allowed group" do
+      get admin_access_path, params: {
         user_id: users(:alice).id,
         application_id: @kavita.id
       }
@@ -25,9 +25,9 @@ module Admin
       assert_match "Administrators", response.body # alice is in admin_group; kavita has admin_group
     end
 
-    test "create returns 'cannot access' with reason when user shares no group with the app" do
+    test "returns 'cannot access' with reason when user shares no group with the app" do
       lonely = User.create!(email_address: "lonely@example.com", password: "password123", skip_auto_assign: true)
-      post admin_access_path, params: {
+      get admin_access_path, params: {
         user_id: lonely.id,
         application_id: @kavita.id
       }
@@ -36,8 +36,8 @@ module Admin
       assert_match "shares no group", response.body
     end
 
-    test "create renders form unchanged when ids are missing" do
-      post admin_access_path, params: {user_id: "", application_id: ""}
+    test "renders form unchanged when ids are missing" do
+      get admin_access_path, params: {user_id: "", application_id: ""}
       assert_response :success
       # No result panel should render. The panel-only phrases:
       refute_match "Granted via", response.body