feat: add uriresolver option #1862

zekth · 2021-12-20T08:29:07Z

What issue does this pull request resolve?

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 · 2022-01-04T14:15:46Z

Any news on this?

epoberezkin · 2022-01-15T09:36:15Z

@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 · 2022-01-15T11:20:02Z

@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 · 2022-01-19T20:08:47Z

looks good - thank you!

Couple questions/comments:

Why does MissingRefError need a reference to resolver? I don't see it used anywhere...
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 · 2022-01-19T22:16:53Z

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 · 2022-01-19T22:21:18Z

I see - thank you!

zekth · 2022-01-20T09:01:11Z

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

mcollina · 2022-01-21T13:36:03Z

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

epoberezkin · 2022-02-04T17:52:15Z

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

epoberezkin · 2022-02-04T18:19:36Z

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 · 2022-02-04T18:20:24Z

Will do ASAP!

Maribel400

😏

epoberezkin · 2022-02-24T07:28:00Z

Bump what? :)

Maribel400 · 2022-03-10T00:28:23Z

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 · 2022-03-10T00:29:18Z

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 · 2022-10-28T16:05:31Z

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 · 2024-03-05T20:46:35Z

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 · 2024-03-05T21:18:42Z

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

fetobasic · 2024-03-06T18:37:26Z

@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 · 2024-03-06T22:24:31Z

Sorry i misunderstood. Yes you're right unfortunately.

feat: add uriresolver option

f68fea2

zekth force-pushed the add-fast-uri branch from afe00ce to f68fea2 Compare December 20, 2021 09:28

mcollina reviewed Dec 20, 2021

View reviewed changes

Merge branch 'master' into add-fast-uri

be025a5

mcollina mentioned this pull request Jan 8, 2022

Use fast-uri with Ajv to speed up cold start fastify/fastify#3607

Closed

2 tasks

Merge branch 'master' into add-fast-uri

377915d

epoberezkin added the needs changes label Jan 15, 2022

zekth added 7 commits January 15, 2022 11:33

fix: review

a2c4926

fix: prettier

3c59a4f

fix: lint

55c6844

fix: param order

206550e

fix: test

c3d1a68

fix: test

4c3d3a6

fix: test

227c060

fix: order

9a2cfda

zekth force-pushed the add-fast-uri branch from bcfc31d to 9a2cfda Compare January 20, 2022 08:51

zekth added 3 commits January 20, 2022 10:01

Merge branch 'master' into add-fast-uri

ef98fc3

feat: bump version

3270453

Merge branch 'add-fast-uri' of github.com:zekth/ajv into add-fast-uri

8d8f977

feat: bump version

97fc1a8

epoberezkin merged commit 0e47ab4 into ajv-validator:master Feb 4, 2022

zekth deleted the add-fast-uri branch February 4, 2022 17:53

Maribel400 reviewed Feb 15, 2022

View reviewed changes

chevett mentioned this pull request Oct 28, 2022

handle update to the MissingRegError constructor ajv-validator/ajv-merge-patch#52

Open

zekth mentioned this pull request Jan 24, 2024

new URL is faster fastify/fast-uri#76

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add uriresolver option #1862

feat: add uriresolver option #1862

zekth commented Dec 20, 2021

mcollina left a comment

zekth commented Jan 4, 2022

epoberezkin commented Jan 15, 2022 •

edited

zekth commented Jan 15, 2022

epoberezkin commented Jan 19, 2022

zekth commented Jan 19, 2022

epoberezkin commented Jan 19, 2022

zekth commented Jan 20, 2022

mcollina commented Jan 21, 2022

epoberezkin commented Feb 4, 2022

epoberezkin commented Feb 4, 2022

zekth commented Feb 4, 2022

Maribel400 left a comment

epoberezkin commented Feb 24, 2022

Maribel400 commented Mar 10, 2022 via email

Maribel400 commented Mar 10, 2022 via email

chevett commented Oct 28, 2022

fetobasic commented Mar 5, 2024 •

edited

zekth commented Mar 5, 2024

fetobasic commented Mar 6, 2024 •

edited

zekth commented Mar 6, 2024

feat: add uriresolver option #1862

feat: add uriresolver option #1862

Conversation

zekth commented Dec 20, 2021

mcollina left a comment

Choose a reason for hiding this comment

zekth commented Jan 4, 2022

epoberezkin commented Jan 15, 2022 • edited

zekth commented Jan 15, 2022

epoberezkin commented Jan 19, 2022

zekth commented Jan 19, 2022

epoberezkin commented Jan 19, 2022

zekth commented Jan 20, 2022

mcollina commented Jan 21, 2022

epoberezkin commented Feb 4, 2022

epoberezkin commented Feb 4, 2022

zekth commented Feb 4, 2022

Maribel400 left a comment

Choose a reason for hiding this comment

epoberezkin commented Feb 24, 2022

Maribel400 commented Mar 10, 2022 via email

Maribel400 commented Mar 10, 2022 via email

chevett commented Oct 28, 2022

fetobasic commented Mar 5, 2024 • edited

zekth commented Mar 5, 2024

fetobasic commented Mar 6, 2024 • edited

zekth commented Mar 6, 2024

epoberezkin commented Jan 15, 2022 •

edited

fetobasic commented Mar 5, 2024 •

edited

fetobasic commented Mar 6, 2024 •

edited