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

Support RJSF Schema validation in strict CSP #3059

Closed
Tracked by #4387
twschiller opened this issue Mar 30, 2022 · 15 comments · Fixed by #7081
Closed
Tracked by #4387

Support RJSF Schema validation in strict CSP #3059

twschiller opened this issue Mar 30, 2022 · 15 comments · Fixed by #7081
Assignees

Comments

@twschiller
Copy link
Contributor

Context:

@fregante
Copy link
Contributor

fregante commented Dec 1, 2022

What do you intend to do here? Should we move RJSF to the sandbox or do you want to propose changes upstream? You had mentioned

  1. Get RJSF to add support for an eval-less validation library (either by asking nicely or by submitting as PR). We use: npmjs.com/package/@cfworker/json-schema. (I had actually looked at AJV previously but didn't use it because of eval)
  2. Move schema validation to the Chrome sandbox: Move template engine and brick evaluation to Chrome sandbox #105. This also would probably require modifying RJSF to support a custom validation function. This approach is nice because it also makes sense to move Nunjucks and JQ over there
  3. Modify AJV to not require Function

I'm asking because some code may not currently allow asynchronous function calls and may need refactoring.
Related:

@twschiller
Copy link
Contributor Author

twschiller commented Dec 1, 2022

Looks like the RJSF team has a v5 in beta that modularized the validation code.

In v4 what we have to deal with is:

In v5, it looks like validation is still not async though 😞 : https://github.com/rjsf-team/react-jsonschema-form/blob/main/packages/validator-ajv6/src/validator.ts#L256

So I think our current bet is to look into using extraErrors. For v4 we also may need to use webpack so swap out some internal implementations

@fregante
Copy link
Contributor

fregante commented Dec 1, 2022

Do you have something in mind with regards to async code? What I'm asking is about eval. By your initial comment it seems that this code is in the underlying AJV package (if I understand that correctly)

@twschiller
Copy link
Contributor Author

twschiller commented Dec 2, 2022

I see us as having two options:

  • Swap out for @cfworker/json-schema which is synchronous. So we'd just need to use Webpack to swap out the Ajv validator function
  • Move Ajv to the Sandbox, which would make form validation asynchronous (because of the async sandbox call)

I have no clue what the functional difference between what @cfworker/json-schema can validate vs. Ajv. I suspect that @cfworker/json-schema is sufficient for our field types in the Form Builder. The only gotcha would be if we need to translate the validation exceptions from @cfworker/json-schema to the format RJSF expects from Ajv

Any thoughts on what might be path of least resistance?

@fregante
Copy link
Contributor

fregante commented Dec 2, 2022

I haven’t looked into either dependency yet, but I think that making changes to our own engine is quicker than dealing with upstream PRs and error translation issues that might crop up over time.

  • Is async validation possible/ideal within PB?
  • Do you think it’s practical to make all those calls async?
  • And especially: are we doing any validation in the background? If so, we can only fix the problem by avoiding eval altogether since the sandbox isn’t yet available there. That means: upstream PR or webpack swap.

The ideal solution might be to send a PR to RJSF but the level of effort, lobbying and time is probably high.

@twschiller
Copy link
Contributor Author

twschiller commented Dec 2, 2022

The ideal solution might be to send a PR to RJSF but the level of effort, lobbying and time is probably high.

A PR to introduce @cfworker/json-schema support, or a PR to introduce async validation support?

Async validation support might already be supported enough for what we need: rjsf-team/react-jsonschema-form#1444

They'd potentially accept a PR to introduce @cfworker/json-schema for the upcoming v5 version of rjsf. Or given that they modularized validation in v5, they may choose not to include it in the repository. I think there's little chance they'd prioritize a PR for v4

Do you think it’s practical to make all those calls async?

I think we'd definitely try to use the extraErrors feature. By practical, you're referring to implementation effort of swapping out sync for async, correct? Or are you referring to runtime performance

are we doing any validation in the background?

To my knowledge we aren't. I also poked around and don't see any places.

The only place I could see us using it in the future, is for validating configuration vs. the integration definition schema for assist with debugging. For that we could definitely use @cfworker/json-schema

@fregante

This comment was marked as outdated.

@fregante

This comment was marked as outdated.

@fregante
Copy link
Contributor

fregante commented Dec 2, 2022

I'm learning about this as we go, so further questions may be best discussed with someone more familiar with validation.

In the meantime I opened an issue upstream. Here's my current thought with our 4 options (pick one):

v4

  1. use the sandbox: impossible due to internal RJSF sync call
  2. use ajv via webpack: doable but not trouble-free, especially when we update dependencies

v5

  1. plug @cfworker/json-schema locally via Decouple schema validation from core rjsf-team/react-jsonschema-form#2693
  2. send a PR to make validation async + using the sandbox locally
    • this requires lobbying and a lot of work upstream and locally, I think

@twschiller
Copy link
Contributor Author

Let's not let this block the Sandbox work for Nunjucks, etc.

I think for Option 2 you meant to say "use @cfworker/json-schema" via webpack. Given the the stability of RJSF v4 (because of the impending v5 release), I suspect dependency updates wouldn't be a problem

The next step is to see how close @cfworker/json-schema's output format is to ajv and what any gaps in validation would be. Since we could use this library with both RJSF v4 and v5.

@fregante
Copy link
Contributor

fregante commented Dec 6, 2022

I think for Option 2 you meant to say "use @cfworker/json-schema" via webpack.

Yes 😅 👍

@fregante
Copy link
Contributor

fregante commented Dec 13, 2022

It seems that we don't have any Jest tests around this component so iterating on a webpack-based replacement might be slow.

Given that RJSF authors seem open to the idea of a @cfworker/json-schema plugin for v5:

  • do you want to wait for v5 before doing any work here?
  • should we attempt to use the v5 with ajv immediately to get a head star?
  • do you think we could implement a proper @cfworker/json-schema plugin for v5 from the ground up and send a PR? It might require more work than a webpack-based swap, but it would be easier to test in their own suite, it would be permanent and they'd maintain it afterwards.

@twschiller
Copy link
Contributor Author

twschiller commented Dec 13, 2022

do you think we could implement a proper @cfworker/json-schema plugin for v5 from the ground up and send a PR

I think it's work spending an hour seeing what would be involved in that. I suspect it won't be a major lift given that @cfworker/json-schema is tested against the https://github.com/json-schema-org/JSON-Schema-Test-Suite JSON Schema spec test suite (so adapter-like things must exist)

Once we have a v5 plugin PR, we can in parallel:

  • Choose to do the v4 switch out, and/or
  • Wait for RJSF v5 to GA

However, let's get Nunjucks into the Sandbox prior to doing RJSF work

It seems that we don't have any Jest tests around this component

🤦 We'll want to address that. I suspect it's due to the iframe/communication. We have an SDET Intern starting early Jan. That could b a good component for them to look at the mocking of

@fregante
Copy link
Contributor

fregante commented Dec 10, 2023

Edit: I don’t think so.


Is this relevant to us?

That PR closed this issue:

However I don’t think we can pre-compile the schemas outside the extension… or can we? Maybe pre-compile them in the sandbox and then use them synchronously via ajv8:

Example:

<CompileSchema schema={schema}>
	{compiledSchema =>  <JsonSchemaForm schema={compiledSchema}/>}
</CompileSchema>

This might be easier than replacing the validator:

@fregante fregante mentioned this issue Dec 10, 2023
2 tasks
@fregante
Copy link
Contributor

On further review, that probably won't work. The compilation isn't the issue, it's the validation that can't run in the content script.

  1. Compilation generates JS (can be done even with CSP)
  2. That generated JS needs to be evaluated by RJSF (blocked by CSP)

If async validation was possible, this would be a useful API, but RJSF does not support that.

grahamlangford added a commit that referenced this issue Dec 21, 2023
* [WIP]: switch form validator to cfworker

* Default to draft 7

* update package-lock

* wip

* wip

* wip

* code cleanup

* more code cleanup

* unskips remaining tests

* fixes schema is not extensible issue

* cloneDeep schema for other RJSF forms

* Update src/bricks/transformers/ephemeralForm/EphemeralForm.tsx

Co-authored-by: Ben Loe <below413@gmail.com>

* Update src/components/formBuilder/preview/FormPreview.tsx

Co-authored-by: Ben Loe <below413@gmail.com>

* remove unnecessary comments

* removes unused method args

* removes unnecessary comments

---------

Co-authored-by: Graham Langford <grahamlangford87@gmail.com>
Co-authored-by: Graham Langford <30706330+grahamlangford@users.noreply.github.com>
Co-authored-by: Ben Loe <below413@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants