Store tokens and client secrets as digests (D1 of 3: read by digest, plaintext fallback)

app/controllers/my/access_tokens_controller.rb

@@ -8,29 +8,24 @@ def index
     @access_tokens = my_access_tokens.order(created_at: :desc)
   end
 
-  def show
-    @access_token = my_access_tokens.find(verifier.verify(params[:id]))
-  rescue ActiveSupport::MessageVerifier::InvalidSignature
-    redirect_to my_access_tokens_path, alert: "Token is no longer visible"
-  end
-
   def new
     @access_token = my_access_tokens.new
   end
 
+  # The raw token is shown in this response and on no page after it, so the
+  # response must not be cached.
   def create
-    access_token = my_access_tokens.create!(access_token_params)
+    @access_token = my_access_tokens.create!(access_token_params)
+    no_store
 
     respond_to do |format|
-      format.html do
-        expiring_id = verifier.generate access_token.id, expires_in: 10.seconds
-        redirect_to my_access_token_path(expiring_id)
-      end
+      format.turbo_stream
+      format.html { render :create, status: :created }
 
       format.json do
         render status: :created, json: \
-          { id: access_token.id, token: access_token.token, description: access_token.description,
-            permission: access_token.permission, created_at: access_token.created_at.utc }
+          { id: @access_token.id, token: @access_token.raw_token, description: @access_token.description,
+            permission: @access_token.permission, created_at: @access_token.created_at.utc }
       end
     end
   end
@@ -52,8 +47,4 @@ def my_access_tokens
     def access_token_params
       params.expect(access_token: %i[ description permission ])
     end
-
-    def verifier
-      Rails.application.message_verifier(:access_tokens)
-    end
 end

app/controllers/oauth/authorizations_controller.rb

@@ -5,6 +5,8 @@ class Oauth::AuthorizationsController < Oauth::BaseController
   # An S256 challenge is the unpadded base64url SHA-256 of the verifier: 43 characters (RFC 7636 §4.2).
   S256_CODE_CHALLENGE = /\A[A-Za-z0-9_-]{43}\z/
   RETURN_URL_MAXIMUM_LENGTH = 2000
+  # Read to find the client and the redirect target, so refused before either.
+  LOOKUP_PARAMETERS = %w[ client_id redirect_uri ]
 
   # Browsers apply form-action to every hop of a form submission, redirects
   # included, so the app-wide form-action 'self' would block the consent POST's
@@ -16,12 +18,14 @@ class Oauth::AuthorizationsController < Oauth::BaseController
   # Gated ahead of sign-in so a dark server never asks anyone to sign in for
   # it. A denial is never gated: it only tells the client no.
   before_action :require_issuance_enabled, unless: :denial?
+  before_action :require_single_valued_parameters
   before_action :save_oauth_return_url
   before_action :require_authentication
   disallow_oauth_grants
 
   before_action :set_client
   before_action :validate_redirect_uri
+  before_action :validate_single_valued_parameters
   before_action :validate_response_type
   before_action :validate_pkce
   before_action :validate_scope
@@ -128,12 +132,16 @@ def redirect_with_error(error, description)
       if params[:redirect_uri].present? && @client&.allows_redirect?(params[:redirect_uri]) && @client.vetted_redirect?(params[:redirect_uri])
         redirect_to error_redirect_uri(error, description), allow_other_host: true
       else
-        @error = error
-        @error_description = description
-        render :error, status: :bad_request
+        render_error error, description
       end
     end
 
+    def render_error(error, description)
+      @error = error
+      @error_description = description
+      render :error, status: :bad_request
+    end
+
     def success_redirect_uri(code)
       build_redirect_uri params[:redirect_uri],
         code: code,
@@ -169,6 +177,25 @@ def validated_redirect_form_action_source
       nil
     end
 
+    # client_id and redirect_uri are refused before the client is looked up,
+    # and shown here, never redirected: a malformed redirect_uri is exactly the
+    # one that can't be validated.
+    def require_single_valued_parameters
+      if (malformed = malformed_parameter_names & LOOKUP_PARAMETERS).any?
+        render_error "invalid_request", malformed_parameters_description(malformed)
+      end
+    end
+
+    # The rest are refused once the redirect target is known, so they go back
+    # to a vetted client like any other error.
+    def validate_single_valued_parameters
+      malformed = malformed_parameter_names
+
+      if !performed? && malformed.any?
+        redirect_with_error "invalid_request", malformed_parameters_description(malformed)
+      end
+    end
+
     def build_redirect_uri(base, **query_params)
       uri = URI.parse(base)
       query = URI.decode_www_form(uri.query || "")

app/controllers/oauth/base_controller.rb

@@ -9,7 +9,56 @@ class Oauth::BaseController < ApplicationController
   # a halted chain skips after_actions (RFC 6749 §5.1, §5.2).
   prepend_before_action :prevent_caching, :prevent_referrer
 
+  # OAuth parameters are top-level names. Rails' JSON parameter wrapping would
+  # also copy a JSON body under the controller's name, and for the token
+  # endpoint that's `token`, an OAuth parameter of its own.
+  wrap_parameters false
+
+  # OAuth parameters that name one value (RFC 6749 §3.1: "Request and
+  # response parameters MUST NOT be included more than once"). Rails turns a
+  # name[]= form field or a JSON array or object into an Array or a nested
+  # Parameters, and handed to a lookup such as find_by(client_id: ...), an
+  # Array becomes an SQL IN list. Each endpoint refuses that shape right after
+  # its availability gate, so a dark server still answers 404, and before it
+  # reads anything. The authorization endpoint refuses client_id and
+  # redirect_uri that early, and the rest once its redirect target is known. client_secret isn't here: Oauth::ClientAuthentication reads
+  # it as a credential, and one that isn't a string fails authentication
+  # (invalid_client) like any wrong secret.
+  SINGLE_VALUED_PARAMETERS = %w[
+    client_id redirect_uri scope state
+    code code_verifier code_challenge code_challenge_method response_type
+    grant_type refresh_token token token_type_hint
+    resource subject_token subject_token_type requested_token_type audience account_id
+  ].freeze
+
   private
+    def require_single_valued_parameters
+      if (malformed = malformed_parameter_names).any?
+        refuse_malformed_parameters malformed
+      end
+    end
+
+    # A JSON null is left to each endpoint, which treats it as absent or, for a
+    # refresh's scope, as naming no scope.
+    def malformed_parameter_names
+      (SINGLE_VALUED_PARAMETERS - list_valued_parameter_names).select do |name|
+        params.key?(name) && !params[name].nil? && !params[name].is_a?(String)
+      end
+    end
+
+    # Parameters an endpoint takes as a list, exempt from the check above.
+    def list_valued_parameter_names
+      []
+    end
+
+    def refuse_malformed_parameters(names)
+      oauth_error "invalid_request", malformed_parameters_description(names)
+    end
+
+    def malformed_parameters_description(names)
+      "#{names.to_sentence} must #{names.one? ? "be" : "each be"} a single string"
+    end
+
     # Discovery follows acceptance (see Oauth::Availability): a dark server
     # answers as if it had no authorization server at all.
     def require_acceptance_enabled
@@ -35,9 +84,12 @@ def piloting_client_id
     end
 
     # The metadata issuer and the iss authorization response parameter must be
-    # identical (RFC 9207 §2). script_name: nil keeps an account prefix out of it.
+    # identical (RFC 9207 §2), and clients compare them to the issuer they were
+    # configured with as exact strings (RFC 8414 §3.3). Like bc3's
+    # canonical_issuer_url, it's the origin with no trailing slash:
+    # https://app.fizzy.do. script_name: nil keeps an account prefix out of it.
     def oauth_issuer
-      root_url(script_name: nil)
+      root_url(script_name: nil).delete_suffix("/")
     end
 
     def prevent_caching

app/controllers/oauth/clients_controller.rb

@@ -6,6 +6,7 @@ class Oauth::ClientsController < Oauth::BaseController
   # that counted requests first would answer 429 to the eleventh, not 404.
   before_action :require_issuance_enabled
   rate_limit to: 10, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
+  before_action :require_single_valued_parameters
   before_action :validate_redirect_uris
   before_action :validate_redirect_uri_origins
   before_action :validate_auth_method
@@ -30,6 +31,16 @@ def piloting_client_id
       nil
     end
 
+    # Registration metadata may give scope as a list (validated_scopes), and
+    # its other lists, redirect_uris and grant_types, aren't single-valued names.
+    def list_valued_parameter_names
+      %w[ scope ]
+    end
+
+    def refuse_malformed_parameters(names)
+      oauth_error "invalid_client_metadata", malformed_parameters_description(names)
+    end
+
     # Bounded before any URI is parsed, so an oversized list costs nothing more
     # than reading the request.
     def validate_redirect_uris
@@ -100,8 +111,8 @@ def validated_scopes
     def dynamic_client_registration_response(client)
       {
         client_id: client.client_id,
-        client_secret: client.client_secret,
-        client_secret_expires_at: (0 if client.client_secret),
+        client_secret: client.raw_client_secret,
+        client_secret_expires_at: (0 if client.raw_client_secret),
         client_name: client.name,
         redirect_uris: client.redirect_uris,
         token_endpoint_auth_method: client.token_endpoint_auth_method,

app/controllers/oauth/revocations_controller.rb

@@ -21,6 +21,7 @@ class Oauth::RevocationsController < Oauth::BaseController
   # throttled like the token endpoint (RFC 6749 §2.3.1, RFC 7009 §5).
   rate_limit to: 20, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
 
+  before_action :require_single_valued_parameters
   before_action :reject_ambiguous_client_credentials
   before_action :require_token
 
@@ -42,7 +43,7 @@ def require_token
     end
 
     def revocable_token
-      Identity::AccessToken.personal.find_by(token: params[:token]) || client_token
+      Identity::AccessToken.personal.find_by_raw_token(params[:token]) || client_token
     end
 
     # Either token of a grant revokes the whole grant, and so does a refresh
@@ -51,8 +52,8 @@ def client_token
       if client = authenticated_client
         tokens = Identity::AccessToken.oauth.where(oauth_client: client)
 
-        tokens.find_by(token: params[:token]) || tokens.find_by(refresh_token: params[:token]) ||
-          tokens.find_by(id: Oauth::RetiredRefreshToken.where(refresh_token: params[:token]).select(:access_token_id))
+        tokens.find_by_raw_token(params[:token]) || tokens.find_by_refresh_token(params[:token]) ||
+          tokens.find_by(id: Oauth::RetiredRefreshToken.find_by_raw_token(params[:token])&.access_token_id)
       end
     end
 end

app/controllers/oauth/tokens_controller.rb

@@ -8,6 +8,7 @@ class Oauth::TokensController < Oauth::BaseController
   # that counted requests first would answer 429 to the eleventh, not 404.
   before_action :require_issuance_enabled
   rate_limit to: 20, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
+  before_action :require_single_valued_parameters
   before_action :validate_grant_type
 
   # Authenticate the client the request names before looking anything up about
@@ -54,7 +55,7 @@ def create
       answer_refresh_replay @retired_refresh_token
     elsif @access_token.refresh(permission: @refresh_permission)
       render json: refresh_response(@access_token)
-    elsif retired_refresh_token = Oauth::RetiredRefreshToken.find_by(refresh_token: params[:refresh_token])
+    elsif retired_refresh_token = Oauth::RetiredRefreshToken.find_by_raw_token(params[:refresh_token])
       # A concurrent refresh won the rotation: this is the same retry.
       answer_refresh_replay retired_refresh_token
     else
@@ -132,7 +133,7 @@ def set_identity
     # known to be the grant's own.
     def set_refreshable_access_token
       @access_token = Identity::AccessToken.find_by_refresh_token(params[:refresh_token]) ||
-        (@retired_refresh_token = Oauth::RetiredRefreshToken.find_by(refresh_token: params[:refresh_token]))&.access_token
+        (@retired_refresh_token = Oauth::RetiredRefreshToken.find_by_raw_token(params[:refresh_token]))&.access_token
 
       unless @access_token
         oauth_error "invalid_grant", "Invalid refresh token"
@@ -214,7 +215,7 @@ def authenticate_client
     def answer_refresh_replay(retired_refresh_token)
       if grant = Identity::AccessToken.uncached { Identity::AccessToken.find_by(id: retired_refresh_token.access_token_id) }
         if retired_refresh_token.retryable?(grant)
-          answer_refresh_retry grant
+          answer_refresh_retry grant, retired_refresh_token.successor
         elsif retired_refresh_token.within_grace?
           oauth_error "invalid_grant", "Refresh token superseded"
         else
@@ -228,12 +229,18 @@ def answer_refresh_replay(retired_refresh_token)
 
     # A retry repeats the rotation it lost the response to, so it must ask for
     # what that rotation produced. One asking for less can't be handed the
-    # wider successor that already exists.
-    def answer_refresh_retry(grant)
-      if @refresh_permission == grant.permission
+    # wider successor that already exists. The answer is the very pair that
+    # rotation issued, kept encrypted for the grace window, since only its
+    # digests are stored. Once that's gone there's nothing to repeat, and the
+    # client, which holds no live token, is refused without revoking.
+    def answer_refresh_retry(grant, successor)
+      if @refresh_permission != grant.permission
+        oauth_error "invalid_scope", "Requested scope differs from the rotation being retried"
+      elsif successor
+        grant.assign_attributes raw_token: successor[:access_token], raw_refresh_token: successor[:refresh_token]
         render json: refresh_response(grant)
       else
-        oauth_error "invalid_scope", "Requested scope differs from the rotation being retried"
+        oauth_error "invalid_grant", "Refresh token no longer retryable"
       end
     end
 
@@ -243,10 +250,10 @@ def refresh_response(access_token)
 
     def token_response(access_token, scope: nil)
       {
-        access_token: access_token.token,
+        access_token: access_token.raw_token,
         token_type: "Bearer",
         expires_in: access_token.expires_in,
-        refresh_token: access_token.refresh_token,
+        refresh_token: access_token.raw_refresh_token,
         scope: scope
       }.compact
     end