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

#105 Add sandbox execution; move JQ to it #4701

Merged
merged 28 commits into from
Dec 12, 2022
Merged

#105 Add sandbox execution; move JQ to it #4701

merged 28 commits into from
Dec 12, 2022

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Nov 25, 2022

I'm just running some tests for the sandbox:

  • Verify that the sandbox loads
  • Set up sandbox loading from content script
  • Set up communication via postMessage
  • Set up nunjucks API
  • Set up jq API
  • Test these two APIs
  • Handle errors?
  • Make it safer via MessageChannel
  • Add tests
  • Update tests
  • Decide what to do when a message is sent before the sandbox is ready

I'm tempted to add postMessage support to the Messenger, but I'm afraid to make it even more complicated (including its semi-manual testing). I had done some work in:

src/tinyPages/sandbox.js Outdated Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
src/contentScript/sidebarDomControllerLite.ts Outdated Show resolved Hide resolved
@twschiller
Copy link
Contributor

Cool! I didn't realize that the sandbox is inserted into a page vs. a single sandbox (similar to the singleton background page)

I'm tempted to add postMessage support to the Messenger, but I'm afraid to make it even more complicated (including its semi-manual testing)

Agree that for now using the Messenger isn't important given that there's only 1-3 message types we'll be using for sandbox (render template, validation JSON Schema form, potentially run jq). The main thing is to be able to wrap in a async interface to simplify calling code

Copy link
Contributor Author

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Messaging also set up, 80%. Current result:

Screen Shot 1

src/sandbox/messenger/api.ts Outdated Show resolved Hide resolved
src/sandbox/messenger/api.ts Outdated Show resolved Hide resolved
src/sandbox/messenger/registration.ts Outdated Show resolved Hide resolved
src/utils/postMessage.ts Fixed Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 26, 2022

Codecov Report

Merging #4701 (8e763be) into main (b4efabb) will increase coverage by 0.04%.
The diff coverage is 70.73%.

@@            Coverage Diff             @@
##             main    #4701      +/-   ##
==========================================
+ Coverage   53.79%   53.84%   +0.04%     
==========================================
  Files         935      941       +6     
  Lines       27766    27846      +80     
  Branches     5365     5369       +4     
==========================================
+ Hits        14937    14993      +56     
- Misses      12829    12853      +24     
Impacted Files Coverage Δ
src/contentScript/contentScriptCore.ts 0.00% <0.00%> (ø)
src/sandbox/messenger/api.ts 0.00% <0.00%> (ø)
src/sandbox/messenger/registration.ts 0.00% <0.00%> (ø)
src/sandbox/sandbox.ts 0.00% <0.00%> (ø)
src/utils/shadowWrap.ts 0.00% <ø> (ø)
src/sandbox/messenger/executor.ts 44.44% <44.44%> (ø)
src/utils.ts 66.79% <66.66%> (-0.01%) ⬇️
src/utils/postMessage.ts 93.93% <93.93%> (ø)
src/blocks/transformers/jq.ts 93.33% <100.00%> (ø)
src/utils/injectIframe.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vercel
Copy link

vercel bot commented Dec 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
pixiebrix-extension ✅ Ready (Inspect) Visit Preview Dec 6, 2022 at 10:49AM (UTC)

Copy link
Contributor Author

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ready for a first review and live testing with JQ.

Before merge:

  • Do extended testing of the JQ brick — it seems to work but does it match the previous expectations?
  • Update tests

/* webpackChunkName: "jq-web" */ "jq-web"
);

return jq.promised.json(input, filter);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Live testing:

Screen Shot 7

);

nunjucks.configure({ autoescape });
return nunjucks.renderString(template, context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status:

  • implemented safer response mechanism
  • added tests
  • added sandbox mock/bypass for non-sandbox tests
  • sandboxed JQ seems to work in MV2
  • the JQ brick does not seem to work in MV3 still, due to unsafe-eval, potentially due to nunjucks

What's left?

  • Do you have any ready-made JQ blueprints that you could run to ensure they actually work (in MV2)?
  • I may have somehow broken useUndo.test.ts locally. On CI the tests silently fail altogether, Jest may be going in a loop somehow

*
* Prior art: https://groups.google.com/a/chromium.org/g/chromium-extensions/c/IPJSfjNSgh8/m/Dh35-tZPAgAJ
* Relevant discussion: https://github.com/w3c/webextensions/issues/78
*/
Copy link
Contributor Author

@fregante fregante Dec 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long comment FYI

@fregante fregante marked this pull request as ready for review December 11, 2022 18:17
@github-actions
Copy link

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@twschiller
Copy link
Contributor

image

@twschiller twschiller merged commit f427bbc into main Dec 12, 2022
@twschiller twschiller deleted the F/mv3/sandbox branch December 12, 2022 20:52
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.

Move template engine and brick evaluation to Chrome sandbox
2 participants