format: ensure ascii in uri, uri-reference

[bernhardreiter]

• Add test cases for uri and iri validation.
• Change validateURI() to additionally check that all chars are ASCII.
• Add additionally check for disallowed ASCII chars for uri and uri-reference.
• Introduce validateIRI() without ASCII range check

resolve #225

[tschmidtb51]

@bernhardreiter Maybe you should add a iri.json as test file as well.

[bernhardreiter]

Maybe you should add a iri.json as test file as well.

I did, but forgot to add it to git. 🤦
Thanks for noticing!

Guess it makes sense to fix this for uri-reference as well.

[s-l-teichmann]

Maybe use if strings.ContainsFunc(s, func(r rune) bool { return r > unicode.MaxASCII }) { instead?

[bernhardreiter]

Tried it, I'll find:

        if strings.ContainsFunc(s, func(r rune) bool { return r > unicode.MaxASCII }) {
            return LocalizableError("has unescaped non-ASCII characters")
        }

slightly less readable (but this may be a matter of not being accustomed to it) than your original code from gocsaf/csaf#517 . Is it more efficient?

[bernhardreiter]

@s-l-teichmann told me that the new code is better in two regards: it works on runes, not on bytes only and it matches a common go style that has come up in the last 1.5 years.

So I'll improve the pull request at this point.

[santhosh-tekuri]

strings.ContainsAny(s, "<>" {}|\^`")

only check of non-ascii; if some one wants strict checking let them implement it;

the changes proposed are adding so many functions and confusing the the readability
I suggest to add single function: ensureASCII(func(v any) error) func(v any) error

and change the initializers as below:

	"uri":                   {"uri", ensureASCII(validateURI)},
	"uri-reference":         {"uri-reference", ensureASCII(validateURIReference)},

[bernhardreiter]

strings.ContainsAny(s, "<>" {}|\\^`")

only check of non-ascii

only checking for non-ascii would remove a check for an illegal \ which is in place in validateURIReference(). Do I understand you correctly that this check for the illegal ASCII char \ should be removed?

if some one wants strict checking let them implement it

(We would like better checking, yes.)

the changes proposed are adding so many functions and confusing the the readability

One function is removed, so instead of two larger functions there is one with four code paths. And four selector functions. I can see that the names are too close to each other.

Adding only ensureASCII would lead to code duplication, as you would need validateURIReference again and the check for s, ok := v.(string) at the beginning of all three functions.

[santhosh-tekuri]

from the point this library implementation: uri, iri validations are same, similarly urn-reference, ire-reference validations are same.

the intension of this PR is to add additional validation for uri, and uri-reference that all characters must be ascii.

so just adding ensureASCII and wrapping uri and url-reference validations with ensureASCII makes it easier to understand.

it seems you are confused that ensureASCII means only alphabets.

[bernhardreiter]

from the point this library implementation: uri, iri validations are same, similarly urn-reference, ire-reference validations are same.

I know that this has been the case, but as RFC 3986 and 3987 are different and https://json-schema.org/draft/2020-12/json-schema-validation#name-resource-identifiers points to them specifically for uri and iri. So a correct validation must take the differences into account.

URIs can only consist out of ASCII chars and in addition some ASCII chars are not allowed (by the ABNF specification). Go's url.Parse does check for a number of invalid chars (like control characters), but not for the chars I've listed in the PR.

The code before was inconsistent in that only one invalid char was tested (\) and only for uri-reference, while it is also an invalid char for uris.

Considering the most correct, consistent and without much duplication code, brought me to the current revision. :)

[tschmidtb51]

LGTM

[santhosh-tekuri]

seems there is some confusion. I am talking in context of only this PR. This PR is just to add ascii check. if you want other validations do it another PR.

also we are not going into the rabbit hole of implementing entire RFC. if that is the case I suggest to implement as separate library and plugin the validation.

my intension is to make atomic change which just addresses the current PR.

[bernhardreiter]

@santhosh-tekuri thanks for the further explanation.

I will think about how to split up the changes - even it makes the between two changes less consistent.

Can you also clarify:

also we are not going into the rabbit hole of implementing entire RFC

Disallowing illegal ASCII characters in format uri and uri-refence seems a good check according to the JSON Schema definition. More checks would be possible, especially considering schema, but I agree that it would be a little bit too much to include here. So I'd recommend doing the illegal ASCII chars validation. I assume that is okay for you. If not - you would need to remove the stray test for \.

[santhosh-tekuri]

I will think about how to split up the changes - even it makes the between two changes less consistent.

we already have parseURL which is superset of uri and iri. just add parseURI which adds any additional validations for uri and uni-reference

if you are willing I suggest to create PR to https://github.com/json-schema-org/JSON-Schema-Test-Suite to add your extra tests.

[koplas]

I submitted a pull request with the additional tests: json-schema-org/JSON-Schema-Test-Suite#778.

[koplas]

There is already a test file in the json schema test suite: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/48461fc3568972801b40eaccc428a31bce338f6e/tests/draft-next/optional/format/iri-reference.json. Maybe we should use this as it contains more tests.