Add Grape::Mountable marker to identify a Grape app (#2793)

* Add Grape::Mountable marker to identify a Grape app

Grape distinguished a mounted Grape app from a bare Rack app by duck-typing
on an incidental internal method (`respond_to?(:inheritable_setting)`).
That is brittle: Grape::API and Grape::API::Instance share no common
ancestor and don't even respond to the same methods (mount_instance vs
inheritable_setting/endpoints), so there was no single predicate and the
checks keyed on a settings method that has nothing to do with mountability.

Introduce a Grape::Mountable marker extended by both Grape::API and
Grape::API::Instance, giving one explicit `is_a?(Grape::Mountable)`
predicate. Route the identity checks through it: Endpoint#bare_rack_app?
(used for forward_match) and the mount branch in DSL::Routing.

Capability checks that go on to call a stage-specific method are left as
respond_to? on purpose (a Mountable does not respond to every such method):
`respond_to?(:mount_instance)` still gates the remountable-class stage,
`respond_to?(:endpoints)` still guards reading endpoints, and the settings
inheritance check still guards calling inheritable_setting.

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

* Harden mounted-app auth surface and error HTML escaping

Two security-relevant fixes surfaced by a review of Grape::API:

- Grape::Middleware::Error#rack_response only escaped the error message
  when the response content-type was exactly 'text/html', so a
  parameterized value such as 'text/html; charset=utf-8' skipped escaping
  and could reflect an unescaped message into an HTML response. Compare the
  media type only.

- A bare Rack app mounted with `mount` is called directly and never goes
  through the endpoint middleware stack, so an API's authentication
  middleware does not wrap it and the mount is reachable unauthenticated.
  Mounted Grape APIs are unaffected. Warn at compile time when a bare Rack
  app is mounted under configured authentication so the bypass isn't silent.

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

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@dblock.org>

Author: 3 people

CHANGELOG.md

@@ -12,6 +12,7 @@
 * [#2782](https://github.com/ruby-grape/grape/pull/2782): Remove the dead `format` keyword from `Grape::Router::Pattern` - [@ericproulx](https://github.com/ericproulx).
 * [#2783](https://github.com/ruby-grape/grape/pull/2783): Read a route's `version`, `anchor` and `requirements` from its pattern instead of storing them again on the route - [@ericproulx](https://github.com/ericproulx).
 * [#2784](https://github.com/ruby-grape/grape/pull/2784): Move HEAD route creation into `Grape::Router::Route#to_head` - [@ericproulx](https://github.com/ericproulx).
+* [#2793](https://github.com/ruby-grape/grape/pull/2793): Add a `Grape::Mountable` marker to identify a Grape app instead of duck-typing on `respond_to?(:inheritable_setting)` - [@ericproulx](https://github.com/ericproulx).
 * [#2792](https://github.com/ruby-grape/grape/pull/2792): Move `Grape::Path` to `Grape::Router::Pattern::Path` and add a `Grape::Router::Pattern.build` factory (`Grape::Path` kept as a deprecated constant) - [@ericproulx](https://github.com/ericproulx).
 * [#2785](https://github.com/ruby-grape/grape/pull/2785): Make route `params` a first-class endpoint input and split `Grape::Router::Route#params` into `#params` (declared definitions) and `#params_for(input)` (extracted values) - [@ericproulx](https://github.com/ericproulx).
 * [#2786](https://github.com/ruby-grape/grape/pull/2786): Make route `requirements` and `anchor` explicit keyword arguments and first-class endpoint inputs instead of opaque `route_options` keys - [@ericproulx](https://github.com/ericproulx).
@@ -20,6 +21,7 @@
 #### Fixes
 
 * [#2767](https://github.com/ruby-grape/grape/pull/2767): Update rubocop to 1.88.0 and rubocop-rspec to 3.10.2 - [@ericproulx](https://github.com/ericproulx).
+* [#2789](https://github.com/ruby-grape/grape/pull/2789): Escape error messages served with a parameterized `text/html` content-type (e.g. `text/html; charset=utf-8`) and warn when a bare Rack app is mounted under authentication - [@ericproulx](https://github.com/ericproulx).
 * [#2790](https://github.com/ruby-grape/grape/pull/2790): Stop mutating `Grape::Router`'s internal maps at request time for HTTP methods with no routes (data race on JRuby/TruffleRuby and unbounded growth) - [@ericproulx](https://github.com/ericproulx).
 * [#2791](https://github.com/ruby-grape/grape/pull/2791): Build HEAD/OPTIONS/405 helper routes from a copy of the inheritable settings instead of temporarily mutating the shared class-level settings during compilation - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.

lib/grape/api.rb

@@ -4,6 +4,9 @@ module Grape
   # The API class is the primary entry point for creating Grape APIs. Users
   # should subclass this class in order to build an API.
   class API
+    # Marks this and every subclass as a mountable Grape app (see Grape::Mountable).
+    extend Grape::Mountable
+
     # Class methods that we want to call on the API rather than on the API object
     NON_OVERRIDABLE = %i[base= base_instance? call change! configuration compile! inherit_settings recognize_path reset! routes top_level_setting= top_level_setting].freeze
 

lib/grape/api/instance.rb

@@ -5,6 +5,8 @@ class API
     # The API Instance class, is the engine behind Grape::API. Each class that inherits
     # from this will represent a different API instance
     class Instance
+      # Marks this and every mounted instance as a mountable Grape app (see Grape::Mountable).
+      extend Grape::Mountable
       extend Grape::DSL::Settings
       extend Grape::DSL::Desc
       extend Grape::DSL::Validations

lib/grape/content_types.rb

@@ -22,7 +22,19 @@ def content_types_for(from_settings)
     def mime_types_for(from_settings)
       return MIME_TYPES if from_settings == Grape::ContentTypes::DEFAULTS
 
-      from_settings.invert.transform_keys! { |k| k.include?(';') ? k.split(';', 2).first : k }
+      from_settings.invert.transform_keys! { |k| media_type(k) }
+    end
+
+    # The media type of a content-type header: the part before any `;`
+    # parameters, with surrounding whitespace removed
+    # (e.g. `'text/html'` for `'text/html; charset=utf-8'`). Returns nil for a
+    # nil content type. Skips the split (and its allocation) when there are no
+    # parameters, which is the common case.
+    def media_type(content_type)
+      return if content_type.nil?
+
+      base = content_type.include?(';') ? content_type.split(';', 2).first : content_type
+      base.strip
     end
   end
 end

lib/grape/dsl/routing.rb

@@ -133,7 +133,9 @@ def mount(mounts, *opts)
           end
           in_setting = inheritable_setting
 
-          if app.respond_to?(:inheritable_setting, true)
+          # Past the mount_instance branch above, a Grape app here is an already
+          # instantiated Grape::API::Instance (vs. a bare Rack app).
+          if app.is_a?(Grape::Mountable)
             mount_path = Grape::Util::PathNormalizer.call(path)
             app.top_level_setting.namespace_stackable[:mount_path] = mount_path
 

lib/grape/endpoint.rb

@@ -276,6 +276,7 @@ def instrument_run_filters(filters, type, &)
 
     def compile!
       @app = config.app || build_stack
+      warn_unauthenticated_mounted_app
       @helpers = build_helpers
       stackable = inheritable_setting.namespace_stackable
       @befores            = stackable[:befores]
@@ -290,7 +291,7 @@ def to_routes
       route_options = config.route_options
       params = config.params
       path_settings = prepare_default_path_settings
-      forward_match = config.app && !config.app.respond_to?(:inheritable_setting)
+      forward_match = bare_rack_app?
       version = prepare_version(inheritable_setting.namespace_inheritable[:version])
       prefix = inheritable_setting.namespace_inheritable[:root_prefix]
       requirements = prepare_routes_requirements(config.requirements)
@@ -313,6 +314,13 @@ def to_routes
       end
     end
 
+    # True when a bare Rack app (anything that isn't a Grape app) is mounted at
+    # this endpoint. Such an app is called directly and matched by path prefix
+    # rather than an anchored route.
+    def bare_rack_app?
+      config.app && !config.app.is_a?(Grape::Mountable)
+    end
+
     def prepare_default_path_settings
       namespace_stackable_hash = inheritable_setting.namespace_stackable.to_hash
       namespace_inheritable_hash = inheritable_setting.namespace_inheritable.to_hash
@@ -391,6 +399,19 @@ def build_helpers
       Module.new { helpers.each { |mod_to_include| include mod_to_include } }
     end
 
+    # A bare Rack app mounted with +mount+ is called directly (see +compile!+):
+    # it does not go through +build_stack+, so the API's authentication
+    # middleware never runs and the mount is reachable unauthenticated. Mounted
+    # Grape APIs are unaffected because they rebuild their own stack from the
+    # inherited settings. Warn so this bypass isn't silent.
+    def warn_unauthenticated_mounted_app
+      return unless bare_rack_app?
+      return unless inheritable_setting.namespace_inheritable[:auth]
+
+      warn "Grape: #{config.app} is mounted under an API that declares authentication, but authentication " \
+           'middleware does not wrap mounted Rack applications. Requests to this mount are not authenticated by Grape.'
+    end
+
     def build_response_cookies
       return unless request.cookies?
 

lib/grape/middleware/error.rb

@@ -58,10 +58,21 @@ def call!(env)
       private
 
       def rack_response(status, headers, message)
-        message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == 'text/html'
+        message = Rack::Utils.escape_html(message) if html_content_type?(headers[Rack::CONTENT_TYPE])
         Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), Grape::Util::Header.new.merge(headers))
       end
 
+      # Escaping must key off the media type only, case-insensitively. Comparing
+      # the raw header against 'text/html' would let a parameterized value such
+      # as 'text/html; charset=utf-8' (or a differently-cased 'Text/HTML', which
+      # browsers still treat as HTML) skip escaping and reflect an unescaped
+      # message into an HTML response. Such a header can be set from several
+      # places (a registered content type, or a custom Content-Type passed to
+      # error!/rescue_from), but they all render here.
+      def html_content_type?(content_type)
+        Grape::ContentTypes.media_type(content_type).to_s.casecmp?('text/html')
+      end
+
       def format_message(error)
         current_format = env[Grape::Env::API_FORMAT] || format
         formatter = Grape::ErrorFormatter.formatter_for(current_format, error_formatters, default_error_formatter)

lib/grape/mountable.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Grape
+  # Marker module for a mountable Grape application. Both {Grape::API} (the
+  # remountable user-facing class) and {Grape::API::Instance} (the compiled
+  # engine) extend it, so a mounted Grape app can be told apart from a bare
+  # Rack app with `is_a?(Grape::Mountable)` — a single, explicit predicate
+  # rather than duck-typing on an incidental internal method such as
+  # `inheritable_setting`.
+  #
+  # `Grape::API` and `Grape::API::Instance` are not related by inheritance and
+  # do not even respond to the same methods (the former to `mount_instance`,
+  # the latter to `inheritable_setting`/`endpoints`), so there is no common
+  # ancestor to key an `is_a?` check on without this marker.
+  #
+  # It answers identity only ("is this a Grape app?"). Capability checks that
+  # go on to call a stage-specific method — e.g. `respond_to?(:endpoints)`
+  # before reading `endpoints` — must stay as they are, since a `Mountable`
+  # does not necessarily respond to every such method.
+  module Mountable
+  end
+end

spec/grape/api_spec.rb

@@ -3600,6 +3600,27 @@ def self.call(object, _env)
       end
     end
 
+    context 'with a bare rack app mounted under authentication' do
+      it 'warns that the mount is not covered by the auth middleware' do
+        subject.http_basic { |_u, _p| false }
+        subject.mount mounted_app => '/mounty'
+        expect { get '/mounty' }.to output(/not authenticated by Grape/).to_stderr
+        expect(last_response.body).to eq('MOUNTED')
+      end
+
+      it 'does not warn for a bare rack app mounted without authentication' do
+        subject.mount mounted_app => '/mounty'
+        expect { get '/mounty' }.not_to output.to_stderr
+      end
+
+      it 'does not warn for a mounted Grape API under authentication' do
+        inner = Class.new(described_class) { get('/inner') { 'inner' } }
+        subject.http_basic { |_u, _p| false }
+        subject.mount inner => '/sub'
+        expect { get '/sub/inner' }.not_to output.to_stderr
+      end
+    end
+
     describe 'the mounted endpoint' do
       it 'exposes the mounted app through #mounted_app' do
         subject.mount mounted_app => '/mounty'
@@ -4454,6 +4475,40 @@ def before
     end
   end
 
+  context 'when an HTML error content-type carries a charset parameter' do
+    it 'still escapes the reflected message' do
+      subject.content_type :html, 'text/html; charset=utf-8'
+      subject.format :html
+      subject.formatter :html, ->(object, _env) { object.to_s }
+      subject.rescue_from(:all) { |e| error!(e.message, 400) }
+      subject.get('/echo') { raise params[:q] }
+      get '/echo', q: '<script>alert(1)</script>'
+      expect(last_response.status).to eq(400)
+      expect(last_response.body).to eq(Rack::Utils.escape_html('<script>alert(1)</script>'))
+    end
+
+    it 'escapes when the charset content-type is set by error! rather than registered' do
+      subject.format :json
+      subject.rescue_from(:all) { |e| error!(e.message, 400, 'Content-Type' => 'text/html; charset=utf-8') }
+      subject.get('/echo') { raise params[:q] }
+      get '/echo', q: '<script>alert(1)</script>'
+      expect(last_response.status).to eq(400)
+      expect(last_response.headers['content-type']).to eq('text/html; charset=utf-8')
+      expect(last_response.body).not_to include('<script>')
+      expect(last_response.body).to include('&lt;script&gt;')
+    end
+
+    it 'escapes regardless of the media type casing' do
+      subject.format :json
+      subject.rescue_from(:all) { |e| error!(e.message, 400, 'Content-Type' => 'Text/HTML; charset=utf-8') }
+      subject.get('/echo') { raise params[:q] }
+      get '/echo', q: '<script>alert(1)</script>'
+      expect(last_response.status).to eq(400)
+      expect(last_response.body).not_to include('<script>')
+      expect(last_response.body).to include('&lt;script&gt;')
+    end
+  end
+
   context 'with non-UTF-8 characters in specified format' do
     it 'converts the characters' do
       subject.format :json

spec/grape/content_types_spec.rb

@@ -75,4 +75,32 @@
       it { is_expected.to eq('application/xml' => :xml) }
     end
   end
+
+  describe '.media_type' do
+    subject { described_class.media_type(content_type) }
+
+    context 'with a bare media type' do
+      let(:content_type) { 'text/html' }
+
+      it { is_expected.to eq('text/html') }
+    end
+
+    context 'with parameters' do
+      let(:content_type) { 'text/html; charset=utf-8' }
+
+      it { is_expected.to eq('text/html') }
+    end
+
+    context 'with surrounding whitespace before the parameter separator' do
+      let(:content_type) { 'text/html ; charset=utf-8' }
+
+      it { is_expected.to eq('text/html') }
+    end
+
+    context 'when nil' do
+      let(:content_type) { nil }
+
+      it { is_expected.to be_nil }
+    end
+  end
 end