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

feature: use constructed style sheets for dom render so that it works with csp style-src 'self' #4611

Merged
merged 4 commits into from
Jul 27, 2023
Merged

feature: use constructed style sheets for dom render so that it works with csp style-src 'self' #4611

merged 4 commits into from
Jul 27, 2023

Conversation

SimonSiefke
Copy link
Contributor

@SimonSiefke SimonSiefke commented Jul 27, 2023

Fixes #4445

This changes the dom renderer to use constructed stylesheets instead of html style elements when available. Constructed stylesheets don't require a style-src 'unsafe-inline' Content Security Policy

test the change locally

  1. In demo/client.ts comment out the webgl addon so that the dom renderer is used
 // if (addons.webgl.instance) {
  //   try {
  //     typedTerm.loadAddon(addons.webgl.instance);
  //     term.open(terminalContainer);
  //     setTextureAtlas(addons.webgl.instance.textureAtlas);
  //     addons.webgl.instance.onChangeTextureAtlas(e => setTextureAtlas(e));
  //     addons.webgl.instance.onAddTextureAtlasCanvas(e => appendTextureAtlas(e));
  //     addons.webgl.instance.onRemoveTextureAtlasCanvas(e => removeTextureAtlas(e));
  //   } catch (e) {
  //     console.warn('error during loading webgl addon:', e);
  //     addons.webgl.instance.dispose();
  //     addons.webgl.instance = undefined;
  //   }
  // }
  1. In demo/server.js, add a content security policy header when serving index.html:
app.get("/", (req, res) => {
    res.setHeader(
      "Content-Security-Policy",
      `default-src 'none'; style-src 'self'; img-src 'self'; script-src 'self'; connect-src 'self'`
    );
    res.sendFile(__dirname + "/index.html");
  });

Demo with CSP enabled

csp-header

@Tyriar
Copy link
Member

Tyriar commented Jul 27, 2023

I updated the branch

@SimonSiefke SimonSiefke marked this pull request as ready for review July 27, 2023 15:39
@Tyriar
Copy link
Member

Tyriar commented Jul 27, 2023

This seems to make the Linux/Firefox integration tests consistently fail. You can run these with npm run test-api-firefox

@SimonSiefke
Copy link
Contributor Author

Fixed! :)

@jerch
Copy link
Member

jerch commented Jul 27, 2023

@SimonSiefke Can you plz also check, that replaceSync does not create another forced layouting step, the last thing we need in the DOM renderer is additional render engine work. So the question here is, if that sync in the name means sync to DOM render state.

@Tyriar
Copy link
Member

Tyriar commented Jul 27, 2023

@jerch I get the impression replaceSync is doing what we used to do and replace would batch it for later if it isn't needed immediately like in our tests. But regardless, even slightly slower here shouldn't matter as it's only on dimensions change (not resize) and changing theme which seldom happens. I double checked the time by resizing a few times on my macbook:

Screenshot 2023-07-27 at 12 44 24 pm

@Tyriar Tyriar added this to the 5.3.0 milestone Jul 27, 2023
@Tyriar Tyriar merged commit 5889344 into xtermjs:master Jul 27, 2023
8 checks passed
@SimonSiefke
Copy link
Contributor Author

@jerch As far as I understand, there is no extra synchronization in replaceSync, it is just the synchronous version of replace


Can you plz also check, that replaceSync does not create another forced layouting step


As @Tyriar said, it is basically the same, both methods are dom writes, there are no extra forced layouts with this method.

Perhaps performance might even be slightly better because there are two less dom elements now, which is also something but I doubt it would make a huge difference.

@Tyriar
Copy link
Member

Tyriar commented Jul 28, 2023

@SimonSiefke thanks again, this was a great change as I know several people have hit the CSP issue

@jerch
Copy link
Member

jerch commented Jul 29, 2023

I stumbled over one aspect in this PR when merging the DOM renderer PR with master - in StyleSheet.ts the global document is accessed. In an older PR we abstracted document into a terminal setting to make popouts into other documents working, as some functionality is bound to the real underlying document.

Question - should the code in StyleSheet.ts also refer to our document variable or does it not matter here? Note I had a similar case with cross document canvas painting, but it just works for those across documents.

@Tyriar
Copy link
Member

Tyriar commented Jul 29, 2023

@jerch good find, I'll fix it up

Tyriar added a commit to Tyriar/xterm.js that referenced this pull request Jul 29, 2023
Tyriar added a commit to Tyriar/xterm.js that referenced this pull request Aug 15, 2023
…stylesheets"

This reverts commit 5889344, reversing
changes made to 679c149.
Tyriar added a commit to Tyriar/xterm.js that referenced this pull request Aug 15, 2023
…stylesheets"

This reverts commit 5889344, reversing
changes made to 679c149.
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.

Latest version requires unsafe-inline due to inline styles
3 participants