CSP error from Page Editor analysis rule in MV3

[twschiller]

Describe the bug

The TemplateAnalysis Page Editor analysis rule uses new Template which calls eval under the hood

To Reproduce

1. Load the MV3 extension
2. Add a trigger
3. Add the Show Sidebar brick and click into the template field

Actual behavior

Error shown due to the invalid template

Discussion

We may need to add a sandbox to the devtools to be able to compile the template (preferred?). Or will need to message the content script which then messages its sandbox

Expected behavior

No error

Additional context

• 1.8.6-beta.1

[fregante]

@grahamlangford I think this PR was supposed to fix this issue? Or is it something else?

• #3059: switch RJSF validator to cfworker/jsonschema #7081

We may need to add a sandbox to the devtools to be able to compile the template

Yes it's possible to add a sandbox iframe anywhere (except the MV3 background worker) and it's preferred.

[twschiller]

@grahamlangford I think this PR was supposed to fix this issue? Or is it something else?

This is something else. It's a use on Nunjucks in the analysis:

pixiebrix-extension/src/analysis/analysisVisitors/templateAnalysis.ts

Line 95 in 132c7cc

new Template(expression.__value__, undefined, undefined, true);

The analysis is instantiating the template to determine whether or not it's a valid template

[twschiller]

@grahamlangford I'd recommend bumping this up as it's a blockers to me using the MV3 build as my daily driver

[grahamlangford]

@fregante feel free to pick this up when you're looking for next work, otherwise @BLoe or I will pick it up when one of us finishes up our current issues.

[fregante]

Source of the error:

pixiebrix-extension/src/analysis/analysisVisitors/templateAnalysis.ts

Line 95 in 25dd3d0

new Template(expression.__value__, undefined, undefined, true);

[fregante]

I opened a PR but this requires larger changes that need to be discussed first (visitExpression must be async)

[twschiller]

I opened a PR but this requires larger changes that need to be discussed first (visitExpression must be async)

I would think we'd want to avoid changing the color of the visitExpression method because that will result in every visitor method needing to be async.

This is what we've done for others:

pixiebrix-extension/src/analysis/analysisVisitors/requestPermissionAnalysis.ts

Line 146 in 76ee8fa

await Promise.allSettled(this.permissionCheckPromises);

[fregante]

It's already similar to that but the annotation is not added. What am I doing wrong? The "add annotation" method is called in the async .catch callback but I only see my own logging, not the annotation

[twschiller]

The "add annotation" method is called in the async .catch callback but I only see my own logging, not the annotation

You're missing the await allSettled in the async run method for the analysis. The idea is to keep track of which promises are created in an array and await them being settled

The analysis results go into redux. Changes to the analysis annotation after the fact won't flow through because the Redux state is an immutable copy of the results

[fregante]

Oh yes I remember seeing this being implemented a while ago actually. I'll give it a try