Skip to content

fix(lit): replace unsafeCSS with CSSStyleSheet for structural styles#633

Open
iamrajhans wants to merge 3 commits intogoogle:mainfrom
iamrajhans:fix-lit-unsafe-css-462
Open

fix(lit): replace unsafeCSS with CSSStyleSheet for structural styles#633
iamrajhans wants to merge 3 commits intogoogle:mainfrom
iamrajhans:fix-lit-unsafe-css-462

Conversation

@iamrajhans
Copy link
Contributor

This PR removes unsafeCSS usage from the Lit renderer to fix internal build policy failures reported in issue #462.

Specifically, in renderers/lit/src/0.8/ui/styles.ts, unsafeCSS(Styles.structuralStyles) was replaced with a safer stylesheet construction flow:

  • build a CSSStyleSheet
  • load Styles.structuralStyles using replaceSync
  • return [] as a fallback when CSSStyleSheet is unavailable or fails

This preserves behavior (shared structural styles still applied) while eliminating direct unsafeCSS usage.

Fixes: #462

Pre-launch Checklist

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively addresses the security concern of using unsafeCSS by replacing it with the safer CSSStyleSheet API. The implementation is solid, with a good fallback for environments where CSSStyleSheet is not supported. I have one suggestion to improve debuggability by logging potential errors during stylesheet creation.

Comment on lines 29 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It's good practice to log errors that are caught, even when providing a fallback. Swallowing the error here could make it difficult to debug if new CSSStyleSheet() or replaceSync() fails for an unexpected reason. Adding a console.error would provide visibility into such issues without changing the fallback behavior.

  } catch (e) {
    console.error('Failed to construct structural styles:', e);
    return [];
  }

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good advice, and if we were to keep this behavior of returning an empty array, we should also console.error in the cases where CSSStyleSheet() is not available. (But IMO, we shouldn't)

Copy link
Collaborator

@ditman ditman left a comment

Choose a reason for hiding this comment

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

I left some comments, and I think @ava-cassiopeia should have final say, but this looks good to me! Thanks for the fix!

(I'm also modifying the description of this PR to link it to the issue about removing unsafeCSS)

Comment on lines 21 to 23
Copy link
Collaborator

Choose a reason for hiding this comment

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

The CSSStyleSheet() constructor seems to be very widely supported, I don't think this check is necessary.

(In case we want to keep this, I'd suggest adding a console.error on the cases where we return [] to let the user know why their stylesheets are not working!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

In what cases would this fail? Why would we want this try-catch here? (I don't see any "Exceptions" sections in the docs?)

Comment on lines 29 to 31
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good advice, and if we were to keep this behavior of returning an empty array, we should also console.error in the cases where CSSStyleSheet() is not available. (But IMO, we shouldn't)

Copy link
Collaborator

@ava-cassiopeia ava-cassiopeia left a comment

Choose a reason for hiding this comment

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

This generally LGTM, but I think @ditman has some solid points, so I'll wait to approve until those are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

Remove unsafeCSS

3 participants

Comments