Use Addressable gem to handle URIs #174

RST-J · 2014-11-02T18:15:05Z

This would cover #173
It adds support for Asian URLs.
When I changed the validation for the URI format to Addressable, I thought that the value should not be escaped (it was introduced to establish compatibility with Asian URIs by eca92af). The value either is parsable as URI as is or not and in the latter case no valid URI.
What do you think?

pd · 2014-11-02T18:41:41Z

Gemfile

@@ -3,6 +3,7 @@ source "https://rubygems.org"
 gemspec

 gem "json", :platforms => :mri_18
+gem "addressable", '~> 2.3'


You should add this to the gemspec instead, so that the dependency is picked up when users gem install json-schema.

pd · 2014-11-02T18:58:50Z

Confirmed that this fixes #100 (or, at least, I can run all tests from a dir json schema).

I'm not sure if the chomp code introduced in #109 still needs to be around after this. Maybe! As @iainbeeston mentioned, that might've been entirely an artifact of the URI::File class we had.

For escaping URIs, addressable happily parses them with spaces, but open-uri still can't deal with it unless escaped:

require 'addressable/uri'
require 'open-uri'

unescaped = Addressable::URI.parse('http://foo.com/spaces in/the path#/not/fragment')
escaped = Addressable::URI.parse(Addressable::URI.escape('http://foo.com/spaces in/the path#/not/fragment'))

open(unescaped) #=> URI::InvalidURIError
open(escaped) #=> OpenURI::HTTPError: 404 Not Found

RST-J · 2014-11-02T19:09:12Z

For the { "$ref": "/etc/passwd" } thing, I just tested that and it in deed does as you guessed.
And I do not see a reason to allow absolute file-URIs (also not file:///etc/passwd) to be referenced. If at all this should be something a user can activate via a security option after being informed about the dragons.

For the chomp thing I'll check and remove if its superfluous now.

RST-J · 2014-11-02T22:53:40Z

Loading local files

In the last commit I tried to forbid using local file system references for security reasons. As @pd already said, this would break backwards compatibility (a lot of the reference tests actually rely on loading files from the disk).

I'm not sure how to interpret section 5.27. id from draft-03 in the context of a root schema.
What does

If this schema is not contained in
any parent schema, the current URI of the parent schema is held to be
the URI under which this schema was addressed. If id is missing, the
current URI of a schema is defined to be that of the parent schema.

mean for e.g. this schema, what is the id/URI for the following schema:

    {
      "$schema" => "http://json-schema.org/draft-03/schema#",
      "type" => "object",
      "properties" => {
        "b" => {
          "required" => true
        }
      }
    }

Currently, the best I can think of to not lose backwards compatibility, would be an option that allows loading relative references from disk. This option unfortunately needs to default to true then.
Forbidding absolute paths is not the solution I guess.
A solution could be that if the user wants to allow loading schemas from file then he can specify one or more directories from where (including sub folders) schemas may be loaded.
As this implies again more changes I'd suggest to move usage Addressable to v3.

pd · 2014-11-03T01:56:49Z

I don't think this means we need to hold off on addressable until a 3.x release; it definitely solves problems today, means we could drop the URI::File code, etc. We just shouldn't break filesystem lookups (yet!). The issue with absolute paths already exists on master: JSON::Validator.validate({"$ref" => "/etc/passwd"}, {}).

If you force pushed this branch without 463f0e3, I think it's pretty much good to go.

A few days back I left a comment in #152 with my ideas for how to deal with preventing arbitrary filesystem lookups, accessing remote URIs without meaning to, etc etc. I haven't had a chance to spike a version of that yet, but I think we can get it into 2.x, then swap to the more restrictive default in 3.x.

iainbeeston · 2014-11-03T08:36:02Z

lib/json-schema/attributes/formats/uri.rb

@@ -7,8 +7,8 @@ def self.validate(current_schema, data, fragments, processor, validator, options
        if data.is_a?(String)
          error_message = "The property '#{build_fragment(fragments)}' must be a valid URI"
          begin
-            URI.parse(URI.escape(data))
-          rescue URI::InvalidURIError
+            Addressable::URI.parse(Addressable::URI.escape(data))


Maybe you already explained this in the PR description (I didn't quite follow everything) but do we still need to escape the uri here now we're using addressable?

Looking at the addressable readme, I suspect we need to parse urls from strings, and then normalise them (probably immediately before calling open on them)

For the first comment: We "need" the escape call here, because only 'parse' won't raise the expected expception for line 278 in draft4-tests. I'd also suspect it to not be actually necessary.

Addressable::URI.parse(data)

has the following output

(byebug) Addressable::URI.parse(data) Addressable::URI::InvalidURIError Exception: Cannot assemble URI string with ambiguous path: '::boo' (byebug) Addressable::URI.parse(data).class Addressable::URI (byebug) Addressable::URI.parse(data).to_s Addressable::URI::InvalidURIError Exception: Cannot assemble URI string with ambiguous path: '::boo' nil

You see that there is an exception somewhere, at least the text, but its not thrown unless e.g. to_s is called. I should probably file an issue on this as I'd expect an exception to be thrown by parse right away.

I'll go for to_s for now.

For the second: It depends I guess. File uris are a problem in general so far. open does not like the file:// scheme nor escaped paths. I agree for the parts that are actual URIs but we don't now that before e.g. nested refs have been resolved against their parent scheme's URI.
So I wouldn't do much about this until we decided how to handle files.

iainbeeston · 2014-11-03T08:46:41Z

As @pd said I'd love to see if the chomp from #109 could be removed in this branch.

I also suspect that we should be using normalise (rather than unescape) with addressable... (Might need more investigation)

iainbeeston · 2014-11-03T08:48:08Z

lib/json-schema/validator.rb

-          uri = URI.parse("file://#{Dir.pwd}/#{data}")
-        end
+      uri = Addressable::URI.parse(data)
+      # Check for absolute path


Addressable has a convert_path method that converts relative paths to file URIs :-)

That won't help us much, because open can't handle them. However, when we have a custom loader, I think its good when the scheme is properly set.
I wonder why there is no FileURI neither in the URI nor in Addressable.

Oh what? That was half of my reasoning for using addressable. My bad. Then perhaps the URI::File class should be kept after all.

I side-stepped this in #175 by avoiding the open(foo).read pattern for URIs with nil or file schemes. We shouldn't be using that on files anyhow, since it will leave dangling file descriptors open. eg:

$ pry [1] pry(main)> $PROCESS_ID => 55886 [2] pry(main)> open('/etc/passwd').read => [lol snip] # From another terminal, or after ctrl-z, whatever: $ lsof -p 55886 ruby 55886 pd 8r REG 1,3 5253 515836 /private/etc/passwd

Definitely absurd that open(Addressable::URI.convert_path('/etc/passwd')) fails yet open(Addressable::URI.parse('/etc/passwd')) works fine, though =)

Yes, I also think we should keep URI::File.

What worries me, is that if I google other implementations of URI::File, I can't find any. In fact, only this gem comes up, which makes me wonder why nobody else is doing this

I found a feature reques from a year ago and one comment mentions the URI::File solution.

I also wonder why this topic seems to have few/no attention, but I think URI::File is a better solution than writing a custom wrapper around open (i.e. its the better wrapper).

I disagree; URI::File is the wrong direction to go in. We only use it so that we can keep using open-uri, but we shouldn't be using open-uri to open local files -- there are many more obvious means of doing that (eg, File.read, Pathname#read, etc etc).

We can assign schema URIs as file:///tmp/ohai.json, but when we read that file, it should be using File.read(uri.path) or something similar.

So going back to my original question (if we're not using file uris) should we use convert path here?

I think we should merge this; my Loader PR has additional changes to more aggressively detect files vs remote URIs, so I think it can be taken care of there.

pd · 2014-11-04T23:13:53Z

test/data/file with spaces.json

@@ -0,0 +1,3 @@
+{
+  "name" : "john"


No tests actually hit this file yet, unless I'm overlooking something.

RST-J · 2014-11-07T18:33:45Z

I updated the PR to work along with #177
Just wondering whether we still can open files with spaces in names and whether we did not reintroduce the problem with hash tags in file names on Windows.

iainbeeston · 2014-11-08T07:04:20Z

If we can simplify the code, that would be great. Maybe try removing any workarounds and then see if anyone can test this branch on windows?=

pd · 2014-11-08T17:11:32Z

I can test this on a windows box some time this weekend.

pd · 2014-11-14T19:56:44Z

Aforementioned windows box apparently runs ubuntu now so uhhh, scratch that plan. I have no further access to a windows machine to try this out on. =\

RST-J · 2014-11-22T00:24:18Z

Any more concerns here?

iainbeeston · 2014-11-22T08:14:05Z

lib/json-schema/attributes/formats/uri.rb

-        rescue URI::InvalidURIError
-          error_message = "The property '#{build_fragment(fragments)}' must be a valid URI"
+          # TODO
+          # Addressable only throws an exception on to_s for invalid URI strings, although it


Do we want to fix this todo before merging? If not could the comment link to a github issue in addressable?

I don't see any upstream reports of this. I'm not sure exactly what to report if I were to open an issue. It's partly an artifact of our test data:

Addressable::URI.parse('http:') #=> raises Addressable::URI::InvalidURIError Addressable::URI.parse('::boo') #=> returns a URI Addressable::URI.parse('::boo').to_s #=> raises Addressable::URI::InvalidURIError: Cannot assemble URI string with ambiguous path: '::boo'

addressable has a lot of tests that assert it can raise on .parse() alone, and indeed in most cases it does. I can't look up a URI RFC atm, but is ::boo somehow valid? Is it ambiguous?

Using addressable 2.3.6 I get the following results:

a = Addressable::URI.parse('http:') # raises Addressable::URI::InvalidURIError: Absolute URI missing hierarchical segment: 'http:' # a == nil b = Addressable::URI.parse('::boo') # raises nothing but returns Addressable::URI::InvalidURIError # hence, b holds a Addressable::URI::InvalidURIError

I'd expect the second statement to also raise an exception but in no case to return one.
I'll go open an issue in addressable (sporkmonger/addressable#177).

So for now I think we should leave this as is.

iainbeeston · 2014-11-22T08:29:08Z

I'd like to see this merged, once all concerns from the comments are sorted out. I think that if we can 1. Get rid of some hacky fixes we have for unusual urls (because of limitations in URI), and 2. Get better url parsing, then this is the way to go

pd · 2014-11-22T17:23:11Z

@RST-J -- you'll need to rebase to deal with merge conflicts.

Just got ruby+git+etc booted on a windows VM and the test suite is green, including from dirs with spaces in the path. Really wish we had some way to run tests on windows that wasn't so insane =)

Use Addressable gem to handle URIs

To ensure that invalid uris of the form "::http" throw exceptions rather than failing silently. This aught to make uri parsing more robust See sporkmonger/addressable#177 and voxpupuli#174

pd reviewed Nov 2, 2014
View reviewed changes

iainbeeston reviewed Nov 3, 2014
View reviewed changes

pd mentioned this pull request Nov 3, 2014

JSON::Schema::Reader #175

Merged

RST-J force-pushed the addressable branch from 463f0e3 to cad37b9 Compare November 4, 2014 21:56

pd reviewed Nov 4, 2014
View reviewed changes

test/data/file with spaces.json

@@ -0,0 +1,3 @@

{

"name" : "john"

Copy link

Contributor

pd Nov 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests actually hit this file yet, unless I'm overlooking something.

pd mentioned this pull request Nov 7, 2014

Refactor ref schema URI construction. #177

Merged

RST-J force-pushed the addressable branch from cad37b9 to c442f1d Compare November 7, 2014 18:31

RST-J force-pushed the addressable branch from 5d82bd6 to 3758f1d Compare November 16, 2014 14:01

iainbeeston reviewed Nov 22, 2014
View reviewed changes

RST-J added 6 commits November 24, 2014 20:08

Use Addressable gem for URI handling

3635e43

Test against maliciuous relative links

b08746d

Explicitly tell rubies about UTF-8

e9e0f9d

Move addressable dependency to gemspec

3201df3

Use to_s to trigger URI validation

655c9a0

Adjustments to refactored URI building/fetching

46d0908

RST-J added 3 commits November 24, 2014 20:10

Use File.read for local files

bbb756e

Add test for file uris with spaces and fragment

0bdc8d3

Use convert path. Load file:// schema with File.read

13eba8e

RST-J force-pushed the addressable branch from 3758f1d to 13eba8e Compare November 24, 2014 23:14

Also use convert_path for absolute URIs

5e8b7a3

pd added a commit that referenced this pull request Nov 25, 2014

Merge pull request #174 from RST-J/addressable

7f4682b

Use Addressable gem to handle URIs

pd merged commit 7f4682b into voxpupuli:master Nov 25, 2014

This was referenced Nov 25, 2014

Use Addressable for handling URIs? #173

Closed

When extending a schema in folder names with spaces #100

Closed

RST-J mentioned this pull request Nov 25, 2014

Use unescape for paths before reading files #188

Merged

iainbeeston mentioned this pull request Nov 30, 2014

Addressable spring clean #199

Merged

iainbeeston mentioned this pull request Feb 6, 2015

Updated to addressable 2.3.7 #226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Addressable gem to handle URIs #174

Use Addressable gem to handle URIs #174

RST-J commented Nov 2, 2014

pd Nov 2, 2014

pd commented Nov 2, 2014

RST-J commented Nov 2, 2014

RST-J commented Nov 2, 2014

pd commented Nov 3, 2014

iainbeeston Nov 3, 2014

iainbeeston Nov 3, 2014

RST-J Nov 4, 2014

RST-J Nov 4, 2014

iainbeeston commented Nov 3, 2014

iainbeeston Nov 3, 2014

RST-J Nov 4, 2014

iainbeeston Nov 4, 2014

pd Nov 4, 2014

RST-J Nov 4, 2014

iainbeeston Nov 4, 2014

RST-J Nov 4, 2014

pd Nov 4, 2014

iainbeeston Nov 22, 2014

pd Nov 22, 2014

pd Nov 4, 2014

RST-J commented Nov 7, 2014

iainbeeston commented Nov 8, 2014

pd commented Nov 8, 2014

pd commented Nov 14, 2014

RST-J commented Nov 22, 2014

iainbeeston Nov 22, 2014

pd Nov 22, 2014

RST-J Nov 24, 2014

iainbeeston commented Nov 22, 2014

pd commented Nov 22, 2014

Use Addressable gem to handle URIs #174

Use Addressable gem to handle URIs #174

Conversation

RST-J commented Nov 2, 2014

Choose a reason for hiding this comment

pd commented Nov 2, 2014

RST-J commented Nov 2, 2014

RST-J commented Nov 2, 2014

Loading local files

pd commented Nov 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainbeeston commented Nov 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RST-J commented Nov 7, 2014

iainbeeston commented Nov 8, 2014

pd commented Nov 8, 2014

pd commented Nov 14, 2014

RST-J commented Nov 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainbeeston commented Nov 22, 2014

pd commented Nov 22, 2014