Only rescue errors explicitly

There are a lot of places in the code where we rescue any error, at all. That's quite dangerous, as it could conceal bugs. I've changed it so that we always specify which error class we want to catch. Mostly it's been a minor change, but when it comes to loading data I've had to introduce an explicit list of protocols which we can load json over (otherwise it's possible to have a uri with a scheme that makes no sense - it'd still be a valid uri but loading from it is impossible). I've used the list of protocols that addressable recognizes for now.
voxpupuli · Mar 6, 2015 · f0d72fd · f0d72fd
1 parent 8497d4c
commit f0d72fd
Show file tree

Hide file tree

Showing 8 changed files with 58 additions and 33 deletions.
diff --git a/lib/json-schema/attributes/formats/date.rb b/lib/json-schema/attributes/formats/date.rb
@@ -11,7 +11,8 @@ def self.validate(current_schema, data, fragments, processor, validator, options
           if REGEXP.match(data)
             begin
               Date.parse(data)
-            rescue Exception
+            rescue ArgumentError => e
+              raise e unless e.message == 'invalid date'
               validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
             end
           else

diff --git a/lib/json-schema/attributes/formats/date_time.rb b/lib/json-schema/attributes/formats/date_time.rb
@@ -14,18 +14,16 @@ def self.validate(current_schema, data, fragments, processor, validator, options
 
             begin
               Date.parse(parts[0])
-            rescue Exception
+            rescue ArgumentError => e
+              raise e unless e.message == 'invalid date'
               validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
               return
             end
 
-            begin
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[1].to_i > 23
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[2].to_i > 59
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[3].to_i > 59
-            rescue Exception
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
-            end
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m.length < 4
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[1].to_i > 23
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[2].to_i > 59
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[3].to_i > 59
           else
             validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
           end

diff --git a/lib/json-schema/attributes/not.rb b/lib/json-schema/attributes/not.rb
@@ -17,7 +17,7 @@ def self.validate(current_schema, data, fragments, processor, validator, options
             message = "The property '#{build_fragment(fragments)}' of type #{data.class} matched the disallowed schema"
             failed = false
           end
-        rescue
+        rescue ValidationError
           # Yay, we failed validation.
         end
 

diff --git a/lib/json-schema/errors/json_load_error.rb b/lib/json-schema/errors/json_load_error.rb
@@ -0,0 +1,6 @@
+module JSON
+  class Schema
+    class JsonLoadError < StandardError
+    end
+  end
+end
diff --git a/lib/json-schema/errors/schema_parse_error.rb b/lib/json-schema/errors/schema_parse_error.rb
@@ -0,0 +1,6 @@
+module JSON
+  class Schema
+    class SchemaParseError < StandardError
+    end
+  end
+end
diff --git a/lib/json-schema/util/uri.rb b/lib/json-schema/util/uri.rb
@@ -1,6 +1,8 @@
 module JSON
   module Util
     module URI
+      SUPPORTED_PROTOCOLS = Addressable::URI::PORT_MAPPING.keys.freeze
+
       def self.normalized_uri(uri)
         uri = Addressable::URI.parse(uri) unless uri.is_a?(Addressable::URI)
         # Check for absolute path

diff --git a/lib/json-schema/validator.rb b/lib/json-schema/validator.rb
@@ -9,6 +9,8 @@
 
 require 'json-schema/schema/reader'
 require 'json-schema/errors/schema_error'
+require 'json-schema/errors/schema_parse_error'
+require 'json-schema/errors/json_load_error'
 require 'json-schema/errors/json_parse_error'
 
 module JSON
@@ -54,17 +56,13 @@ def initialize(schema_data, data, opts={})
 
       # validate the schema, if requested
       if @options[:validate_schema]
-        begin
-          if @base_schema.schema["$schema"]
-            base_validator = JSON::Validator.validator_for_name(@base_schema.schema["$schema"])
-          end
-          metaschema = base_validator ? base_validator.metaschema : validator.metaschema
-          # Don't clear the cache during metaschema validation!
-          meta_validator = JSON::Validator.new(metaschema, @base_schema.schema, {:clear_cache => false})
-          meta_validator.validate
-        rescue JSON::Schema::ValidationError, JSON::Schema::SchemaError
-          raise $!
+        if @base_schema.schema["$schema"]
+          base_validator = JSON::Validator.validator_for_name(@base_schema.schema["$schema"])
         end
+        metaschema = base_validator ? base_validator.metaschema : validator.metaschema
+        # Don't clear the cache during metaschema validation!
+        meta_validator = JSON::Validator.new(metaschema, @base_schema.schema, {:clear_cache => false})
+        meta_validator.validate
       end
 
       # If the :fragment option is set, try and validate against the fragment
@@ -520,7 +518,7 @@ def initialize_schema(schema)
             schema = schema.to_array_schema
           end
           Validator.add_schema(schema)
-        rescue
+        rescue JSON::ParserError
           # Build a uri for it
           schema_uri = Util::URI.normalized_uri(schema)
           if !self.class.schema_loaded?(schema_uri)
@@ -551,7 +549,7 @@ def initialize_schema(schema)
         end
         Validator.add_schema(schema)
       else
-        raise "Invalid schema - must be either a string or a hash"
+        raise SchemaParseError, "Invalid schema - must be either a string or a hash"
       end
 
       schema
@@ -567,12 +565,12 @@ def initialize_data(data)
         elsif data.is_a?(String)
           begin
             data = JSON::Validator.parse(data)
-          rescue
+          rescue JSON::ParserError
             begin
               json_uri = Util::URI.normalized_uri(data)
               data = JSON::Validator.parse(custom_open(json_uri))
-            rescue
-              # Silently discard the error - the data will not change
+            rescue Schema::JsonLoadError
+              # Silently discard the error - use the data as-is
             end
           end
         end
@@ -582,10 +580,18 @@ def initialize_data(data)
 
     def custom_open(uri)
       uri = Util::URI.normalized_uri(uri) if uri.is_a?(String)
-      if uri.absolute? && uri.scheme != 'file'
-        open(uri.to_s).read
+      if uri.absolute? && Util::URI::SUPPORTED_PROTOCOLS.include?(uri.scheme)
+        begin
+          open(uri.to_s).read
+        rescue OpenURI::HTTPError, Timeout::Error => e
+          raise Schema::JsonLoadError, e.message
+        end
       else
-        File.read(Addressable::URI.unescape(uri.path))
+        begin
+          File.read(Addressable::URI.unescape(uri.path))
+        rescue SystemCallError => e
+          raise Schema::JsonLoadError, e.message
+        end
       end
     end
   end

diff --git a/test/test_initialize_data.rb b/test/test_initialize_data.rb
@@ -14,7 +14,7 @@ def test_parse_character_string
       JSON::Validator.validate(schema, data, :json => true)
     end
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_integer_string
@@ -27,7 +27,7 @@ def test_parse_integer_string
 
     assert(JSON::Validator.validate(schema, data, :json => true))
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_hash_string
@@ -40,7 +40,7 @@ def test_parse_hash_string
 
     assert(JSON::Validator.validate(schema, data, :json => true))
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_json_string
@@ -53,7 +53,7 @@ def test_parse_json_string
 
     assert(JSON::Validator.validate(schema, data, :json => true))
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_valid_uri_string
@@ -83,11 +83,17 @@ def test_parse_invalid_uri_string
 
     assert(JSON::Validator.validate(schema, data, :parse_data => false))
 
+    stub_request(:get, "foo.bar").to_return(:status => [500, "Internal Server Error"])
+
+    assert(JSON::Validator.validate(schema, data))
+
+    assert(JSON::Validator.validate(schema, data, :parse_data => false))
+
     assert_raises(parser_error) do
       JSON::Validator.validate(schema, data, :json => true)
     end
 
-    assert_raises(Timeout::Error) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_integer