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

#2946 Error thrown on Custom form submit #3019

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

BALEHOK
Copy link
Contributor

@BALEHOK BALEHOK commented Mar 24, 2022

Closes #2946

@twschiller
Copy link
Contributor

twschiller commented Mar 24, 2022

@fregante do you know if there's anyway we can apply the relaxed CSP only to the form page? Maybe not possible in MV2?: https://developer.chrome.com/docs/extensions/mv3/intro/mv3-migration/#content-security-policy

Even in MV3 the only contexts are sandbox vs. extension_pages?

I'd like to avoid relaxing the CSP for all extension pages if possible

@twschiller twschiller added the do not merge Merging of this PR is blocked by another one label Mar 29, 2022
@twschiller
Copy link
Contributor

twschiller commented Mar 29, 2022

It looks like there's no way to relax the CSP on the ephemeralForm.html using meta tags: https://stackoverflow.com/questions/34131814/how-to-relax-content-security-policy-with-meta-tag

However, we could use meta tags to tighten the script CSP on all the other pages?

  • Relax the CSP in manifest.json
  • Tighten the script CSP on the pages that don't need to render a form using meta tags

@fregante thoughts?

@fregante
Copy link
Contributor

fregante commented Mar 30, 2022

I don't know how missed my previous @mention!

I think you're right about this, the problem though is that:

  • to tighten the CSP in the background page, you'd have to load an HTML background page, which is the legacy way to do that. HTML background pages are not possible in MV3, so it'd be a temporary solution
  • I'm not sure if the extension’s CSP applies to the content script, but if it does, I don’t think it can be tightened there

I find it quite limiting that ajv isn't able to do this without resorting to eval, maybe someone is working on a solution to that? Or we can avoid it?

I suppose for now we can merge this and open another ticket to keep track of possible better solutions.

@twschiller
Copy link
Contributor

twschiller commented Mar 30, 2022

I'm not sure if the extension’s CSP applies to the content script, but if it does, I don’t think it can be tightened there

I don't think it does? Otherwise we would have seen this error back when forms were being executed in the content script

I find it quite limiting that ajv isn't able to do this without resorting to eval, maybe someone is working on a solution to that? Or we can avoid it?

Technically the don't use eval, they use function constructor. Same issues, though.

It's not clear what Chrome's MV3 stance will be toward eval for AJV and Nunjucks. (Since it will be decided at CWS submission review time)

Our options:

  1. Get RJSF to add support for an eval-less validation library (either by asking nicely or by submitting as PR). We use: https://www.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

Short-term, I'm OK merging this PR to bring back support for form validation

I suppose for now we can merge this and open another ticket to keep track of possible better solutions.

I created a ticket here

@twschiller twschiller removed the do not merge Merging of this PR is blocked by another one label Mar 30, 2022
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

Successfully merging this pull request may close these issues.

Error thrown on Custom form submit
3 participants