feat: add uriresolver option

[zekth]

What issue does this pull request resolve?

fix: #1844

What changes did you make?

Integrate fast-uri as an optional uri resolver.

Is there anything that requires more attention while reviewing?

Not sure about where to add the tests. Right now i only used the resolve test suite. Is there any other spot where we should consider adding fast-uri in the test suite?

[mcollina]

Good work!

[zekth]

Any news on this?

[epoberezkin]

@zekth - thanks for doing it.

I think using a condition at all call sites is a brittle approach that may lead to missing some call site, particularly in the future changes.

What could be done instead is removing URI import from resolve.ts entirely (and whatever other file that uses it now, if any) and instead putting it into options, similarly to how it was done for RegExp. Then all call sites would be updated to unconditionally use the resolver from the options, whether it's the default one or the one passed by the user.

This would be set up during instance options initialisation, there are two different types for user supplied options (Options) and runtime options (InstanceOptions), in the latter the uriResolver should be a required member, so you won't have to check in each call site if it is present, and just use it directly.

Also it has to be supported in standalone code, same as it was done for RegExp (I think).

I am referring to this PR that added RE2 support - #1684 (it's marked as closed but it was just accidentally merged via another branch, you can see in the master branch how it is done)...

[zekth]

@epoberezkin i addressed the changes needed. Also discovered that i forgot some other changes. If the shape of this PR is ok for you let me know and i'll rebase it properly.

[epoberezkin]

looks good - thank you!

Couple questions/comments:

1. Why does MissingRefError need a reference to resolver? I don't see it used anywhere...
2. getFullPath accepts resolver as the first parameter and resolveUrl as the last - could you please make it the first in both cases? Another option would be just to call them with Ajv context so resolver is there... (e.g. getFullPath.call(this, ...))

[zekth]

1- It's needed because of this: https://github.com/ajv-validator/ajv/pull/1862/files#diff-14042115de8ebf12385d7afecb8ceaafd6cd54b8ae726e5ef5a68509c965755dR10-R11
2 - Sure i'll rework the shape.

[epoberezkin]

I see - thank you!

[zekth]

@epoberezkin i reordered the parameters. Is it ok this way or you'd prefer to pass the complete Ajv context within the functions?

[mcollina]

I think it's better to bump fast-uri to 0.1.0 or even 1.0.0 to simplify maintenance.

[epoberezkin]

@zekth - this is great, sorry for being so slow - going to release it now!

[epoberezkin]

alright, I already pushed the release out, and then it occurred to me that we didn't add any docs for the feature.

@zekth - please add to the options page if you can :)

Thank you!

[zekth]

Will do ASAP!

[Maribel400]

😏

[epoberezkin]

Bump what? :)

[Maribel400]

Vaya usted a chingar a su madre Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows From: ***@***.***> Sent: Wednesday, February 23, 2022 7:27 PM To: ***@***.***> Cc: ***@***.***>; ***@***.***> Subject: Re: [ajv-validator/ajv] feat: add uriresolver option (PR #1862) @Tuneabq commented on this pull request. You like — Reply to this email directly, view it on GitHub<#1862 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXYW5UBJQ4WU4MUQFLAP3D3U4WCOXANCNFSM5KNDC3AA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[Maribel400]

Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows sakese From: Evgeny ***@***.***> Sent: Thursday, February 24, 2022 1:28 AM To: ***@***.***> Cc: ***@***.***>; ***@***.***> Subject: Re: [ajv-validator/ajv] feat: add uriresolver option (PR #1862) Bump what? :) — Reply to this email directly, view it on GitHub<#1862 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXYW5UDEU2E4FH56ZFS3A4LU4XMYXANCNFSM5KNDC3AA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[chevett]

It appears that this is a breaking change? i'm seeing https://github.com/ajv-validator/ajv-merge-patch/blob/master/keywords/add_keyword.js#L22 fail with ajv@8.10+

[fetobasic]

What issue does this pull request resolve?

fix: #1844

What changes did you make?

Integrate fast-uri as an optional uri resolver.

Is there anything that requires more attention while reviewing?

Not sure about where to add the tests. Right now i only used the resolve test suite. Is there any other spot where we should consider adding fast-uri in the test suite?

@zekth @mcollina @epoberezkin Correct me if I'm wrong, but I assume even if you go with the fast-uri option vulnerability scanners like Orca will still pick up the CVE-2021-44906 vulnerability in uri-js dependency (transitive) since there's still an import import type {URIComponents} from "uri-js" on line 3 of resolve.ts??

[zekth]

@fetobasic this is a dev-dependency so it's not applicable as vulnerability

[fetobasic]

@fetobasic this is a dev-dependency so it's not applicable as vulnerability

@zekth Thanks for the quick response, but I meant uri-js not fast-uri. Uri-js is still a regular dependency in Ajv and uri-js has a transitive dependency on Minimist which contains the vulnerability.

I think I answered my own question the fact that uri-js is still a regular dependency for Ajv, scanners will pick it up, even if you avoid using the library by utilizing the optional uriresolver like fast-uri?

[zekth]

Sorry i misunderstood. Yes you're right unfortunately.