fix: implement stylesheet registration and disallow arbitrary functions #3443

jye-sf · 2023-04-04T23:05:45Z

Details

Implements registerStylesheet for marking stylesheets generated by the LWC compiler.
Restricts stylesheet evaluation to only those stylesheets.
Throws an error if arbitrary functions are found in the stylesheet list.

Addresses #3122 and #3156

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

This has the potential to break use cases relying on behaviors from arbitrary functions being executed as a stylesheet function:

For legitimate stylesheets, users have other methods to inject their own styles (<style> tags, adoptedStylesheets, etc).
For other arbitrary uses, they should not be relying on this behavior.
We may evaluate exposing registerStylesheet to components per [HMR] CSS: Provide a mechanism to register stylesheets #1887.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

GUS work item

W-12040411

packages/@lwc/integration-karma/test/api/registerStylesheet/index.spec.js

packages/@lwc/engine-core/src/framework/stylesheet.ts

packages/@lwc/style-compiler/src/serialize.ts

nolanlawson · 2023-04-05T16:17:01Z

packages/@lwc/engine-core/src/framework/stylesheet.ts

@@ -275,3 +281,18 @@ export function createStylesheet(vm: VM, stylesheets: string[]): VNode[] | null
    }
    return null;
 }
+
+const signedStylesheetSet: Set<StylesheetFactory> = new Set();


I always found it odd that we used Maps/Sets for registering components/templates (rather than a WeakMap/WeakSet), but this is fine since today there is no way to GC these anyway.

In the future, if we ever add the ability to GC/evict components/templates/stylesheets, then we can change these to weak collections.

nolanlawson

Side note: it looks like we won't need to update the ACT compiler, because apparently it doesn't emit stylesheets. (At least, our tests in packages/@lwc/integration-karma/test/act/act-components don't have any.) But it might be good to double-check.

This PR largely LGTM, although of course there is the risk of breakage. People really shouldn't be using arbitrary functions-that-return-strings, but of course they might be. The most prudent would be to make this a warning rather than an error, and roll it out with the reporting API first.

It depends on our appetite for risk, and how much breakage we think this will cause. WDYT?

jye-sf · 2023-04-05T18:09:04Z

I'll double check the ACT.

I like the idea of making this a warning first and integrating it into the reporting API as a low-risk rollout. Risk of breakage is fairly low, but I think our appetite for risk is also medium low right now. I'd also like to get Alex's take on our org's overall appetite for risk, so in the meantime, we can use warning+reporting.

nolanlawson · 2023-04-05T18:59:07Z

Sounds good. At the very least, we can get early data from first-party components. I suspect those are the most likely to be exploiting loopholes in LWC like this anyway.

jye-sf · 2023-04-05T20:45:56Z

packages/@lwc/engine-core/src/framework/reporting.ts

@@ -47,6 +48,7 @@ export type ReportingPayloadMapping = {
    [ReportingEventId.NonStandardAriaReflection]: NonStandardAriaReflectionPayload;
    [ReportingEventId.TemplateMutation]: TemplateMutationPayload;
    [ReportingEventId.StylesheetMutation]: StylesheetMutationPayload;
+    [ReportingEventId.UnexpectedStylesheetContent]: BasePayload;


I originally had a payload interface for this, but I don't see a point since we don't need any properties beyond what's provided by BasePayload. We can always add a new payload interface if that ever changes.

jye-sf · 2023-04-05T20:51:28Z

packages/@lwc/engine-core/src/framework/stylesheet.ts

+                    `TypeError: Unexpected LWC stylesheet content found for component <${vm.tagName.toLowerCase()}>.`
+                );
+                report(ReportingEventId.UnexpectedStylesheetContent, {
+                    tagName: vm.tagName.toLowerCase(),


Is there ever any functional difference between vm.elm.tagName and vm.tagName? I noticed we use the former for non-standard-aria and the latter for cross-root-aria and static stylesheet validation

I don't think there's a difference, no. I was probably just being inconsistent. 😅

jye-sf · 2023-04-05T20:54:48Z

Checked the ACT and as far as I could see we don't emit any stylesheets there. I couldn't find any references to stylesheets except for a single tmpl.stylesheets = [];.

nolanlawson · 2023-04-05T21:05:17Z

packages/@lwc/engine-core/src/framework/stylesheet.ts

+                logError(
+                    `TypeError: Unexpected LWC stylesheet content found for component <${vm.tagName.toLowerCase()}>.`
+                );


This error should only be logged in dev mode, and should also only be logged once ideally. I'd use the existing logWarnOnce, or create a new one called logErrorOnce. (In practice, it doesn't matter much whether the text is yellow or red IMO. 😄)

Went with logWarnOnce. This should now be consistent with how we're reporting and logging the other deprecated features.

nolanlawson · 2023-04-05T21:06:03Z

packages/@lwc/engine-core/src/framework/stylesheet.ts

+                report(ReportingEventId.UnexpectedStylesheetContent, {
+                    tagName: vm.tagName.toLowerCase(),
+                });
+                continue;


This continue makes this a functional (breaking) change. My understanding was that we were just going to warn, but still call the stylesheet function?

Also, let's log a GH issue to track converting the warning into an error, then mention it with a TODO comment here. We can add it to the breaking changes wishlist: #2964

My bad, that detail slipped my mind. 👍 on logging a GH issue to convert the warning to error.

To play it safe, can we also add a test to check that the CSS string returned by the arbitrary function is actually inserted into the DOM (e.g. with getComputedStyle)? That would have caught the continue.

nolanlawson

Just a few nitpicks, otherwise everything LGTM.

jye-sf · 2023-04-05T22:49:48Z

TODO: #3447

jye-sf · 2023-04-05T23:04:26Z

packages/@lwc/engine-core/src/patches/detect-non-standard-aria.ts

@@ -62,7 +62,7 @@ function checkAndReportViolation(elm: Element, prop: string, isSetter: boolean,
        if (process.env.NODE_ENV !== 'production') {
            logWarnOnce(
                `Element <${elm.tagName.toLowerCase()}> ` +
-                    (isUndefined(vm) ? '' : `owned by <${vm.elm.tagName.toLowerCase()}> `) +
+                    (isUndefined(vm) ? '' : `owned by <${vm.tagName.toLowerCase()}> `) +


Updated this to be consistent with everywhere else since this is the only place we do vm.elm.tagName.

jye-sf · 2023-04-05T23:05:08Z

packages/@lwc/integration-karma/test/rendering/programmatic-stylesheets/index.spec.js

+
+        // TODO [#3447]: This test verifies existing behavior is maintained and should be removed
+        // once the warnings are converted into errors.
+        it('applies the styles if the arbitrary function returns a valid string', () => {


Added test to verify existing behavior.

… functions (#3443)" This reverts commit c2265c9.

… functions (#3443)" (#3472) This reverts commit c2265c9.

…ns (#3443) * fix: implement stylesheet registration and disallow arbitrary functions * feat: log as error and use reporting API * fix: move to dev-only warning, address PR comments * fix: add test to verify existing behavior is maintained * fix: only import registerStylesheet if new styles were compiled

jye-sf added 2 commits April 4, 2023 15:57

fix: implement stylesheet registration and disallow arbitrary functions

d8a7813

fix: update snapshots

3e57afa

jye-sf requested a review from nolanlawson April 4, 2023 23:05

jye-sf commented Apr 4, 2023

View reviewed changes

packages/@lwc/integration-karma/test/api/registerStylesheet/index.spec.js Outdated Show resolved Hide resolved

packages/@lwc/engine-core/src/framework/stylesheet.ts Show resolved Hide resolved

packages/@lwc/style-compiler/src/serialize.ts Show resolved Hide resolved

nolanlawson reviewed Apr 5, 2023

View reviewed changes

jye-sf added 2 commits April 5, 2023 13:37

feat: log as error and use reporting API

61f71e5

fix: map directly to BasePayload

8bb5918

jye-sf commented Apr 5, 2023

View reviewed changes

fix: use vm.tagName

f5b64d1

jye-sf commented Apr 5, 2023

View reviewed changes

nolanlawson reviewed Apr 5, 2023

View reviewed changes

nolanlawson approved these changes Apr 5, 2023

View reviewed changes

fix: move to dev-only warning, address PR comments

b3d1225

jye-sf mentioned this pull request Apr 5, 2023

Convert warnings logged from Unexpected LWC stylesheet content to errors #3447

Open

fix: add test to verify existing behavior is maintained

836750a

jye-sf commented Apr 5, 2023

View reviewed changes

jye-sf added 2 commits April 6, 2023 09:32

fix: rename ignored file to avoid .gitignore

49df5c0

fix: only import registerStylesheet if new styles were compiled

d724a8a

jye-sf merged commit c2265c9 into master Apr 6, 2023

jye-sf deleted the jye/register-stylesheet branch April 6, 2023 17:50

This was referenced Apr 7, 2023

Misleading error message when passing an invalid stylesheet #3156

Closed

[HMR] CSS: Provide a mechanism to register stylesheets #1887

Open

test: add expected log check #3461

Closed

jye-sf added a commit that referenced this pull request Apr 27, 2023

Revert "fix: implement stylesheet registration and disallow arbitrary…

b31b3f9

… functions (#3443)" This reverts commit c2265c9.

jye-sf mentioned this pull request Apr 27, 2023

Revert "fix: implement stylesheet registration and disallow arbitrary… #3472

Merged

divmain pushed a commit that referenced this pull request Apr 27, 2023

Revert "fix: implement stylesheet registration and disallow arbitrary…

5e9ee50

… functions (#3443)" (#3472) This reverts commit c2265c9.

This was referenced Apr 27, 2023

chore: release v2.45.0 #3475

Merged

chore: release v2.45.0 #3476

Merged

jye-sf mentioned this pull request May 2, 2023

fix: implement stylesheet registration and disallow arbitrary functions #3477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: implement stylesheet registration and disallow arbitrary functions #3443

fix: implement stylesheet registration and disallow arbitrary functions #3443

jye-sf commented Apr 4, 2023 •

edited

Loading

nolanlawson Apr 5, 2023

nolanlawson left a comment

jye-sf commented Apr 5, 2023

nolanlawson commented Apr 5, 2023

jye-sf Apr 5, 2023

jye-sf Apr 5, 2023

nolanlawson Apr 5, 2023

jye-sf commented Apr 5, 2023

nolanlawson Apr 5, 2023

jye-sf Apr 5, 2023

nolanlawson Apr 5, 2023

nolanlawson Apr 5, 2023

jye-sf Apr 5, 2023

nolanlawson Apr 5, 2023 •

edited

Loading

nolanlawson left a comment

jye-sf commented Apr 5, 2023

jye-sf Apr 5, 2023

jye-sf Apr 5, 2023

fix: implement stylesheet registration and disallow arbitrary functions #3443

fix: implement stylesheet registration and disallow arbitrary functions #3443

Conversation

jye-sf commented Apr 4, 2023 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

jye-sf commented Apr 5, 2023

nolanlawson commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jye-sf commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

jye-sf commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jye-sf commented Apr 4, 2023 •

edited

Loading

nolanlawson Apr 5, 2023 •

edited

Loading