Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"format":"uri" still OK after recent changes? #418

Closed
narcoticfresh opened this issue May 3, 2017 · 8 comments
Closed

"format":"uri" still OK after recent changes? #418

narcoticfresh opened this issue May 3, 2017 · 8 comments

Comments

@narcoticfresh
Copy link
Contributor

Hi guys..

First, thanks for the active work on this project, it's very nice to see that much activity going on here ,-)

An update from 5.1.0 to 5.2.0 broke one of our projects unit tests and i'm not sure if our test expectations are wrong - or the changes here ;-)

The schema

Given this simple schema:

{
  "title": "Example",
  "type": "object",
  "properties": {
    "url": {
      "title": "Url",
      "type": [
        "string",
        "null"
      ],
      "format": "uri"
    }
  },
  "additionalProperties": false
}

The problem

Our unit tests expects the string jjj--no-url not to be a valid url.. but apparently now, it is ;-)
This was changed recently in commit 5e92df0

For us, this is kinda of a problem, we don't regard (and who would?) jjj--no-url as a valid url..

So if i pass the string jjj--no-url as an url, it will land in this condition of the if:
https://github.com/justinrainbow/json-schema/blob/5.2.0/src/JsonSchema/Constraints/FormatConstraint.php#L96

The question

Isn't the check to lean? If we have a look at the line:

$validURL = filter_var('scheme://host/' . $element, FILTER_VALIDATE_URL, FILTER_NULL_ON_FAILURE);

So this basically means that any string will pass (as long it doesn't have invalid filename parts) as it gets prefixed with a scheme and a host (thus seen as a relative part)..

I checked this, even the string a will pass.

IMHO, this makes the uri check obsolete and wrong. If we prepend scheme and host for the user to pass the url check, the user really didn't provide a correct url..

So is this what is really expected, that any string passes the uri format?

Thanks ;-)

@narcoticfresh
Copy link
Contributor Author

@erayd
I just seen your commit message regarding that commit.. If i read that part of the RFC at https://tools.ietf.org/html/rfc3986#section-4.2 it specifies the format:

      relative-ref  = relative-part [ "?" query ] [ "#" fragment ]

      relative-part = "//" authority path-abempty
                    / path-absolute
                    / path-noscheme
                    / path-empty

Doesn't relative-part = "//" authority path-abempty mean, that a relative-part must start with an //?

I just cannot get passed the fact that now every string passes as a filename, now also passes as an uri 😢 So there's no way now to validate a proper uri having a scheme and host?

@erayd
Copy link
Contributor

erayd commented May 3, 2017

You're right. The case that led me to make this change actually turned out to be a meta-schema bug; I don't have the handy right now (not at my desk), but will find out for you tomorrow. This change will be reverted.

@erayd
Copy link
Contributor

erayd commented May 3, 2017

*don't have the link handy right now, but will find it tomorrow.

@narcoticfresh
Copy link
Contributor Author

@erayd OK, thanks..

So is the change to remove that if() clause (which i can open a PR for and add a unit test for the bad url) or a bigger change?

As I see, the // case is already covered by the first if clause..

@erayd
Copy link
Contributor

erayd commented May 3, 2017

The change is bigger - relative URI (i.e. URI references) should not be allowed at all for the uri format.

See json-schema-org/JSON-Schema-Test-Suite#177.

@erayd
Copy link
Contributor

erayd commented May 3, 2017

Right, now that I'm back at my desk...

  • The original change I made was to allow the "uri" format to also support URI references.
  • I made this change because the draft-03 & draft-04 meta-schemas declare "id" and "$ref" as "format": "uri", both those fields legitimately contain references, and RFC-3986 describes valid references.
  • The end result was that anything which looks like a valid URI reference was explicitly considered to be valid for the "uri" format.

Following spec issues 310 & 177:

  • "uri" is indeed intended to only support complete, fully-qualified URI.
  • RFC-3986 §1.2.3 says that URI references are not a subset of URI - I did not notice this paragraph when reading the RFC before I made the original change.
  • The draft-03 & draft-04 meta-schemas declare "id" & "$ref" as "format": "uri", but this is a meta-schema bug, and the correct format for those fields should actually be "uri-reference", however that format is not predefined until draft-05 (as uriref).

In order to ensure that the "uri" format behaves as intended, but does not break schema validation or cause testsuite failures, I am intending the following actions:

  • Revert "uri" to only be valid for fully-qualified, non-reference URI.
  • Add a format option for "uri-reference"
  • Intercept draft-03 and draft-04 meta-schemas and change the format definition for "id" & "$ref" at schema load time to correct the meta-schema bug.

@erayd
Copy link
Contributor

erayd commented May 3, 2017

This is fixed in #419. I will backport this to 5.x.x as soon as it's merged.

@erayd
Copy link
Contributor

erayd commented Jun 23, 2017

@narcoticfresh / @bighappyface
Can this issue be closed? #419 was merged and backported over a month ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants