jhthorsen / json-validator

:cop: Validate data against a JSON schema
https://metacpan.org/release/JSON-Validator
56 stars 58 forks source link

Test problem seen when directory name has RFC 3986 reserved chars #105

Closed mohawk2 closed 5 years ago

mohawk2 commented 6 years ago

The "definitions" file has a space here. I am using this to reproduce the problem that shows up (thanks to the call to Mojo::File::realpath) when any character that Mojo::URL percent-encodes is present anywhere in the full directory path. This is because the encoded version is then used as if it is a viable filesystem location, but it is not due to the encoding.

jhthorsen commented 6 years ago

Is this a bug, a fix, or what is this PR? Sorry, it's so long ago, that I can't remember if we discussed this on IRC or something.

mohawk2 commented 6 years ago

This is a test that demonstrates the problem when a file (or a directory that file lives in) has e.g. a space in it. I wanted to ensure you were interested before I worked on a fix for it.

The currently-released code does not work on my "Perl in space" setup, which you can see in the cpantesters report: t/bundle.t and t/load-from-app.t fail. If you're interested in a fix, let me know.

mohawk2 commented 6 years ago

One problem is in Validator.pm line 79:

        return $from if $ref_name =~ m!^$self->{root_schema_url}\#!;

needs to become:

        return $from if $ref_name =~ m!^\Q$self->{root_schema_url}\E\#!;

The load-from-app.t failure is because /api gets turned into \api on Windows, because the current code is treating a URI as though it's a file location.

jhthorsen commented 6 years ago

Could you push a fix as well, and squash the commits into one?

mohawk2 commented 6 years ago

@jhthorsen This is now ready for review, and actually passes its tests on Windows too!

I left it in two commits because the first one seemed genuinely separate though preparatory for the main change. Let me know if you need any further changes.

jhthorsen commented 5 years ago

I think the PR looks very promising! Thanks for the work 👍

jhthorsen commented 5 years ago

Thank you and sorry for the late review 👍

mohawk2 commented 5 years ago

Real life comes first! Thanks for the review which definitely improved things.