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

feat: support Trusted Types for client overlay #4404

Merged
merged 9 commits into from May 4, 2022

Conversation

Siegrift
Copy link
Contributor

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes. LMK if more are needed.

Motivation / Use-Case

Addresses #4400

Breaking Changes

No breaking changes.

This change is noop for applications which do not enforce Trusted Types. Applications which do enforce Trusted Types and use webpack-dev-server can now whitelist webpack-dev-server#overlay policy to make error overlay work. Optionally, users can choose a different name via policyName attribute (but maybe this could change to more verbose trustedTypesPolicyName?).

Additional Info

See #4400

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 24, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Siegrift / name: Emanuel Tesař (fc7962a)

@alexander-akait
Copy link
Member

Thanks for PR, can you accept CLA?

@@ -15,7 +15,7 @@ import createSocketURL from "./utils/createSocketURL.js";
* @property {boolean} hot
* @property {boolean} liveReload
* @property {boolean} progress
* @property {boolean | { warnings?: boolean, errors?: boolean }} overlay
* @property {boolean | { warnings?: boolean, errors?: boolean, policyName?: string }} overlay
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am wondering whether the policyName should be renamed to more verbose trustedTypesPolicyName. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we will have the policyName option here, but I think make sense to rename it to trustedTypesPolicyName, to avoid misleading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c63489b

@@ -23,10 +23,26 @@ let iframeContainerElement;
let containerElement;
/** @type {Array<(element: HTMLDivElement) => void>} */
let onLoadQueue = [];
/** @type {any} */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are TS type definitions located in https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/trusted-types

However, I am not sure if it is worth typing so I've added any for now.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need install them in dev depedencies, because when types is not found it is any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 68d3974

*/
function createContainer(policyName) {
// Enable Trusted Types if they are available in the current browser.
if (window.trustedTypes) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could optionally check whether the policyName is not null. However, I think using trustedTypes whenever they are available is better because:

  1. It is still noop and backwards compatible change for applications which do not enforce Trusted Types
  2. This enables Trusted Types integration without the need to change anything in webpack configuration (which can be hidden, such as in CRA). This means that the only change the application needs to do is whitelist the default policy name.

@@ -1610,6 +1610,10 @@ declare class Server {
negatedDescription: string;
};
};
policyName: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why there are so many unrelated changes in this autogenerated d.ts file. Especially, since I've added just one field.

Please, LMK if I have something wrong in my setup.

Copy link
Member

Choose a reason for hiding this comment

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

It is bug in ts, ignore it

@Siegrift
Copy link
Contributor Author

I also see that there needs to be a documentation update in https://webpack.js.org/configuration/dev-server/#overlay, specifically this file https://github.com/1ilsang/webpack.js.org/blob/master/src/content/configuration/dev-server.mdx#overlay

I guess that is only updated after there is a new release?

Also, please LMK if there are is anything in this PR I've missed :)

@alexander-akait
Copy link
Member

I also see that there needs to be a documentation update in https://webpack.js.org/configuration/dev-server/#overlay, specifically this file https://github.com/1ilsang/webpack.js.org/blob/master/src/content/configuration/dev-server.mdx#overlay

We will open issue in our docs after merge, dont worry

I guess that is only updated after there is a new release?

Yes, I will do release

devServer: {
client: {
overlay: {
policyName: "overlay-policy",
Copy link

Choose a reason for hiding this comment

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

Consider using webpack#dev-overlay or something like this to feature the recommended naming scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8ef1a48

koto
koto previously approved these changes Apr 25, 2022
@alexander-akait
Copy link
Member

@Siegrift frinedly ping

@Siegrift
Copy link
Contributor Author

Sorry, I got a bit busy with work stuff this week. I'll hopefully find some time tomorrow or over the weekend.

Thanks for the ping :)

@alexander-akait
Copy link
Member

Bih thanks, feel free to ping me

function createContainer(trustedTypesPolicyName) {
// Enable Trusted Types if they are available in the current browser.
if (window.trustedTypes) {
overlayTrustedTypesPolicy = window.trustedTypes.createPolicy(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case there is an error creating a Trusted Types policy the overlay is not shown at all.

A different approach would be to capture the exception outside this function (same place we define overlayTrustedTypesPolicy) and show an overlay without using innerHTML showing that the policy creation failed.

However I think having an error in the console is sufficient, since this is a development only feature so we can assume the console is opened anyways.

@Siegrift
Copy link
Contributor Author

Siegrift commented May 1, 2022

I've fixed the comments and rebased on current master and left one more comment.

@alexander-akait
Copy link
Member

@Siegrift Hello, sorry for delay, can you update snapshots?

@Siegrift Siegrift force-pushed the add-tt-support branch 2 times, most recently from 26d875c to d8da5ca Compare May 3, 2022 12:30
@Siegrift
Copy link
Contributor Author

Siegrift commented May 3, 2022

Sorry for not running the tests locally first :/
However, there seem to be some changed snapshots which are unrelated to my feature. I am not sure about that.

@snitin315
Copy link
Member

@Siegrift help output changed because you added client-overlay-trusted-types-policy-name CLI flag, hence need to update the snapshot.

@Siegrift
Copy link
Contributor Author

Siegrift commented May 4, 2022

Thanks, I am having problems passing the tests locally. When I run yarn test:only --updateSnapshot most of the tests pass, but there are sometimes a few unrelated changes (I've reverted those manually). When I try to run the tests now, they seem to remove the basic should output help should generate correct cli flags 1 in webpack5 snapshot which is wrong.

Is there something I am doing wrong?

@snitin315
Copy link
Member

Are you using macOS?

@Siegrift
Copy link
Contributor Author

Siegrift commented May 4, 2022

Yes, the m1 chip.

@snitin315
Copy link
Member

Yeah, help output snapshots are not quite well on macOS, known bug in nodejs it strips the actual output on stdout.

@Siegrift
Copy link
Contributor Author

Siegrift commented May 4, 2022

I'll try to fix the tests on my Linux machine later today. I don't think I'll get anywhere by testing this on macOS :D

@alexander-akait
Copy link
Member

I want to merge it and fix tests in the next PR, because we need to do release today due security problems in portfinder, big thanks for PR

@alexander-akait alexander-akait merged commit 8132e1d into webpack:master May 4, 2022
@Siegrift
Copy link
Contributor Author

Siegrift commented May 4, 2022

No problem, sorry for taking it too long :/

Good luck with the release.

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.

None yet

4 participants