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

#5676: Restore sandbox for nunjucks and handlebars #7086

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Dec 10, 2023

What does this PR do?

This was added last year but was disabled due to issues with JQ (#4964). JQ has since been removed from the sandbox (#6579).

In any case the memory issue didn't appear to be solvable via Transferable objects (because only ArrayBuffer is transferable, and we'd need to duplicate the object into it before transferring it, voiding any possible memory savings)

Discussion

  • Is this all that's needed here?

Demo

Screen.Recording.mov

Future work

Checklist

  • Add tests
  • New files added to src/tsconfig.strictNullChecks.json (if possible)
  • Designate a primary reviewer: @twschiller

@fregante fregante changed the title #5676: Restore sandbox for nunjuks and handlebars #5676: Restore sandbox for nunjucks and handlebars Dec 10, 2023
@twschiller twschiller added the do not merge Merging of this PR is blocked by another one label Dec 11, 2023
@twschiller
Copy link
Contributor

@fregante @grahamlangford marking as "do not merge" because we'll want to save this for 1.8.6 or feature flag it

@twschiller twschiller removed the do not merge Merging of this PR is blocked by another one label Dec 18, 2023
@twschiller
Copy link
Contributor

twschiller commented Dec 18, 2023

I think we're OK to merge this now testing. But if we include in 1.8.6, I'd want to put behind a skunkworks flag

I'm worried about some host pages in the wild somehow mucking with injected iframes: https://pixiebrix.slack.com/archives/C023KL47XV4/p1702844878608309

We might even have to use an offscreen background document in those cases: https://developer.chrome.com/docs/extensions/reference/api/offscreen

cc @grahamlangford

Copy link
Collaborator

@grahamlangford grahamlangford left a comment

Choose a reason for hiding this comment

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

I've created a ticket to hide the sandbox behavior behind a skunkworks flag: #7150

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 twschiller merged commit faa3e3b into main Dec 18, 2023
13 checks passed
@twschiller twschiller deleted the F/mv3/sandbox branch December 18, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Enable Sandbox for template rendering
3 participants