Add OpenAPI3 strict reference checking on schema load (interagent#343)

.gitignore

@@ -8,3 +8,4 @@ Gemfile.lock
 !/examples/heroku_api/Gemfile.lock
 .DS_Store
 /coverage/
+.idea

.ruby-version

@@ -0,0 +1 @@
+2.6.0

README.md

@@ -22,7 +22,7 @@ use Committee::Middleware::RequestValidation, schema_path: 'docs/schema.json', c
 
 This piece of middleware validates the parameters of incoming requests to make sure that they're formatted according to the constraints imposed by a particular schema.
 
-Options and their defaults:
+### Configuration Options
 
 | name | Hyper-Schema | OpenAPI 3 | Description |
 |-----------:|------------:|------------:| :------------ |
@@ -39,6 +39,7 @@ Options and their defaults:
 |optimistic_json| false | false | Will attempt to parse JSON in the request body even without a `Content-Type: application/json` before falling back to other options. |
 |raise| false | false | Raise an exception on error instead of responding with a generic error body. |
 |strict| false | false | Puts the middleware into strict mode, meaning that paths which are not defined in the schema will be responded to with a 404 instead of being run. |
+|strict_reference_validation| always false | false | Raises an exception (`OpenAPIParser::MissingReferenceError`) on middleware load if the provided schema file contains unresolvable references (`$ref:"#/something/not/here"`). Not supported on Hyper-schema parser. Defaults to `false` on OpenAPI3 but will default to `true` in next major version. |
 |ignore_error| false | false | Validate and ignore result even if validation is error. So always return original data. |
 
 Non-boolean options:
@@ -153,7 +154,7 @@ use Committee::Middleware::ResponseValidation, schema_path: 'docs/schema.json'
 
 This piece of middleware validates the contents of the response received from up the stack for any route that matches the JSON Schema. A hyper-schema link's `targetSchema` property is used to determine what a valid response looks like.
 
-Option values and defaults:
+### Configuration Options
 
 | name | Hyper-Schema | OpenAPI 3 | Description |
 |-----------:|------------:|------------:| :------------ |
@@ -162,6 +163,7 @@ Option values and defaults:
 |ignore_error| false | false | Validate and ignore result even if validation is error. So always return original data. |
 |parse_response_by_content_type| false | false | Parse response body to JSON only if Content-Type header is 'application/json'. When false, this always optimistically parses as JSON without checking for Content-Type header. |
 |strict| false | false | Puts the middleware into strict mode, meaning that response code and content type does not defined in the schema will be responded to with a 500 instead of application's status code. |
+|strict_reference_validation| always false | false | Raises an exception (`OpenAPIParser::MissingReferenceError`) on middleware load if the provided schema file contains unresolvable references (`$ref:"#/something/not/here"`). Not supported on Hyper-schema parser. Defaults to `false` on OpenAPI3 but will default to `true` in next major version. |
 
 No boolean option values:
 
@@ -334,14 +336,21 @@ Committee 3.* has many breaking changes so we recommend upgrading to the latest
 
 Important changes are also described below.
 
-
 ### Upgrading from Committee 4.* to 5.*
 
 Committee 5.* has few breaking changes so we recommend upgrading to the latest release on 4.* and fixing any deprecation errors you see before upgrading.
 (Now we doesn't release 5.* yet)
 
 - change `parse_response_by_content_type`'s default value from `false` to `true`.
 
+#### Future Updates (5.*~)
+OpenAPI3 Schema Users: Newly-added `strict_reference_validation` option defaults to `false` if not set.
+From a later major version of Committee, we want to change it to `true` (or remove the option entirely and force
+`true` to be passed to the OpenAPI3 parser (see: https://github.com/interagent/committee/issues/343).
+
+To remove deprecation warnings, pass `strict_reference_validation: true` as an option when loading the middleware
+(because this is about schema parsing/initialization, the setting is valid for both request and response validation).
+
 ### Setting schemas in middleware
 
 Committee 2.* supported setting `schema` to a string or a hash like this:

committee.gemspec

@@ -19,7 +19,7 @@ Gem::Specification.new do |s|
   s.add_dependency "json_schema", "~> 0.14", ">= 0.14.3"
 
   s.add_dependency "rack", ">= 1.5"
-  s.add_dependency "openapi_parser", ">= 0.11.1", "< 1.0"
+  s.add_dependency "openapi_parser", "1.0.0.beta1"
 
   s.add_development_dependency "minitest", "~> 5.3"
   s.add_development_dependency "rack-test", "~> 0.6"

lib/committee/drivers.rb

@@ -20,27 +20,27 @@ def self.driver_from_name(name)
     # load and build drive from JSON file
     # @param [String] schema_path
     # @return [Committee::Driver]
-    def self.load_from_json(schema_path)
-      load_from_data(JSON.parse(File.read(schema_path)), schema_path)
+    def self.load_from_json(schema_path, parser_options = {})
+      load_from_data(JSON.parse(File.read(schema_path)), schema_path, parser_options)
     end
 
     # load and build drive from YAML file
     # @param [String] schema_path
     # @return [Committee::Driver]
-    def self.load_from_yaml(schema_path)
+    def self.load_from_yaml(schema_path, parser_options = {})
       data = YAML.respond_to?(:unsafe_load_file) ? YAML.unsafe_load_file(schema_path) : YAML.load_file(schema_path)
-      load_from_data(data, schema_path)
+      load_from_data(data, schema_path, parser_options)
     end
 
     # load and build drive from file
     # @param [String] schema_path
     # @return [Committee::Driver]
-    def self.load_from_file(schema_path)
+    def self.load_from_file(schema_path, parser_options = {})
       case File.extname(schema_path)
       when '.json'
-        load_from_json(schema_path)
+        load_from_json(schema_path, parser_options)
       when '.yaml', '.yml'
-        load_from_yaml(schema_path)
+        load_from_yaml(schema_path, parser_options)
       else
         raise "Committee only supports the following file extensions: '.json', '.yaml', '.yml'"
       end
@@ -49,9 +49,19 @@ def self.load_from_file(schema_path)
     # load and build drive from Hash object
     # @param [Hash] hash
     # @return [Committee::Driver]
-    def self.load_from_data(hash, schema_path = nil)
+    def self.load_from_data(hash, schema_path = nil, parser_options = {})
       if hash['openapi']&.start_with?('3.0.')
-        openapi = OpenAPIParser.parse_with_filepath(hash, schema_path)
+        # From the next major version, we want to ensure `{ strict_reference_validation: true }`
+        # as a parser option here, but since it may break existing implementations, just warn
+        # if it is not explicitly set. See: https://github.com/interagent/committee/issues/343#issuecomment-997400329
+        opts = parser_options.dup
+        unless opts.key?(:strict_reference_validation)
+          Committee.warn_deprecated('openapi_parser will default to strict reference validation ' +
+                                        'from next version. Pass config `strict_reference_validation: true` (or false, if you must) ' +
+                                        'to quiet this warning.')
+          opts[:strict_reference_validation] = false
+        end
+        openapi = OpenAPIParser.parse_with_filepath(hash, schema_path, opts)
         return Committee::Drivers::OpenAPI3::Driver.new.parse(openapi)
       end
 
@@ -61,6 +71,7 @@ def self.load_from_data(hash, schema_path = nil)
                  Committee::Drivers::HyperSchema::Driver.new
                end
 
+      # TODO: in the future, pass `opts` here and allow optionality in other drivers?
       driver.parse(hash)
     end
   end

lib/committee/middleware/base.rb

@@ -30,11 +30,12 @@ def call(env)
       class << self
         def get_schema(options)
           schema = options[:schema]
-          unless schema
-            schema = Committee::Drivers::load_from_file(options[:schema_path]) if options[:schema_path]
-
-            raise(ArgumentError, "Committee: need option `schema` or `schema_path`") unless schema
+          if !schema && options[:schema_path]
+            # In the future, we could have `parser_options` as an exposed config?
+            parser_options = options.key?(:strict_reference_validation) ? { strict_reference_validation: options[:strict_reference_validation] } : {}
+            schema = Committee::Drivers::load_from_file(options[:schema_path], parser_options)
           end
+          raise(ArgumentError, "Committee: need option `schema` or `schema_path`") unless schema
 
           # Expect the type we want by now. If we don't have it, the user passed
           # something else non-standard in.

test/data/openapi3/invalid_reference.yaml

@@ -0,0 +1,29 @@
+openapi: 3.0.1
+info:
+  version: 1.0.0
+  title: OpenAPI3 Test
+  description: A Sample file
+paths:
+  /characters:
+    get:
+      description: get characters
+      parameters:
+        - name: school_name
+          in: query
+          description: school name to filter by
+          required: false
+          style: form
+          schema:
+            type: array
+            items:
+              type: string
+      responses:
+        '200':
+          # Does not exist, `ThisIsDefined` exists
+          $ref: '#/components/responses/ThisIsNotDefinedAnywhere'
+
+components:
+  responses:
+    # Not used
+    ThisIsDefined:
+      type: object

test/drivers_test.rb

@@ -48,6 +48,19 @@
       assert_kind_of Committee::Drivers::OpenAPI3::Schema, s
     end
 
+    it 'fails to load OpenAPI 3 with invalid reference' do
+      parser_options = { strict_reference_validation: true }
+      assert_raises(OpenAPIParser::MissingReferenceError) do
+        Committee::Drivers.load_from_file(open_api_3_invalid_reference_path, parser_options)
+      end
+    end
+
+    # This test can be removed when the test above (raising on invalid reference) becomes default behavior?
+    it 'allows loading OpenAPI 3 with invalid reference as existing behavior' do
+      s = Committee::Drivers.load_from_file(open_api_3_invalid_reference_path)
+      assert_kind_of Committee::Drivers::OpenAPI3::Schema, s
+    end
+
     it 'errors on an unsupported file extension' do
       e = assert_raises(StandardError) do
         Committee::Drivers.load_from_file('test.xml')

test/middleware/base_test.rb

@@ -119,6 +119,20 @@ def app
       b = Committee::Middleware::Base.new(nil, schema_path: open_api_3_schema_path, parse_response_by_content_type: false)
       assert_kind_of Committee::Drivers::OpenAPI3::Schema, b.instance_variable_get(:@schema)
     end
+
+    it "accepts OpenAPI3 parser config of strict_reference_validation and raises" do
+      assert_raises(OpenAPIParser::MissingReferenceError) do
+        Committee::Middleware::Base.new(nil, schema_path: open_api_3_invalid_reference_path, strict_reference_validation: true)
+      end
+    end
+
+    it "does not raise by default even with invalid reference OpenAPI3 specification" do
+      b = Committee::Middleware::Base.new(nil, schema_path: open_api_3_invalid_reference_path, strict_reference_validation: false)
+      assert_kind_of Committee::Drivers::OpenAPI3::Schema, b.instance_variable_get(:@schema)
+
+      b = Committee::Middleware::Base.new(nil, schema_path: open_api_3_invalid_reference_path)
+      assert_kind_of Committee::Drivers::OpenAPI3::Schema, b.instance_variable_get(:@schema)
+    end
   end
 
   private

test/test_helper.rb

@@ -112,3 +112,7 @@ def open_api_3_coverage_schema_path
 def open_api_3_0_1_schema_path
   "./test/data/openapi3/3_0_1.yaml"
 end
+
+def open_api_3_invalid_reference_path
+  "./test/data/openapi3/invalid_reference.yaml"
+end