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(runtime-dom): trusted types compatibility #10844

Merged
merged 16 commits into from
Aug 2, 2024

Conversation

haoqunjiang
Copy link
Member

@haoqunjiang haoqunjiang commented Apr 30, 2024

This PR introduces a new __VUE_PROD_TRUSTED_TYPES__ feature flag to enable support for Trusted Types. Addressing vuejs/rfcs#614.

For Trusted Types, I created a vue policy to convert the HTML string output from the Vue compiler to the TrustedHTML type.
This implementation works with the current enforcement of Google Workspace products without any issue.

In some rare circumstances, where the user explicitly configured allowed policy names and not allowing duplicates (Content-Security-Policy: trusted-types vue <their-custom-policy>; without a trailing allow-duplicates), a second version of Vue on the same web page would fail to create the built-in trusted types policy.
This may be solved by allowing users to customize the built-in policy name, but I don't think it's worth the complexity at this moment, considering the low adoption rate of this feature.

Note that the in-browser compiler doesn't benefit from this PR because it's already incompatible with many content security policies anyway.

Remaining todos:

Copy link

github-actions bot commented Apr 30, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 95.5 kB (+5.4 kB) 36.4 kB (+1.89 kB) 32.8 kB (+1.68 kB)
vue.global.prod.js 153 kB (+6.36 kB) 56.2 kB (+2.17 kB) 49.9 kB (+1.89 kB)

Usages

Name Size Gzip Brotli
createApp 53.2 kB (+3.51 kB) 20.7 kB (+1.21 kB) 18.9 kB (+1.07 kB)
createSSRApp 57.1 kB (+3.88 kB) 22.3 kB (+1.35 kB) 20.3 kB (+1.24 kB)
defineCustomElement 55.4 kB (+3.5 kB) 21.5 kB (+1.23 kB) 19.5 kB (+1.08 kB)
overall 66.7 kB (+3.56 kB) 25.7 kB (+1.22 kB) 23.4 kB (+1.13 kB)

@yyx990803
Copy link
Member

replaceChildren seems to require browser support that is above ES2016 baseline.

@haoqunjiang haoqunjiang changed the title fix: ensure no trusted types violations on fresh mount fix: ensure no trusted types violations by default May 7, 2024
@haoqunjiang haoqunjiang changed the title fix: ensure no trusted types violations by default feat: ensure no trusted types violations by default May 8, 2024
@haoqunjiang haoqunjiang marked this pull request as ready for review May 8, 2024 07:56
@haoqunjiang haoqunjiang requested a review from yyx990803 May 8, 2024 07:56
@haoqunjiang haoqunjiang changed the title feat: ensure no trusted types violations by default feat: ensure no trusted types violations May 8, 2024
@haoqunjiang haoqunjiang changed the title feat: ensure no trusted types violations feat: ensure no trusted types violations when __VUE_PROD_TRUSTED_TYPES__ enabled May 8, 2024
@haoqunjiang haoqunjiang changed the title feat: ensure no trusted types violations when __VUE_PROD_TRUSTED_TYPES__ enabled feat(runtime-dom): a new __VUE_PROD_TRUSTED_TYPES__ flag for trusted types compatibility May 8, 2024
@lukewarlow
Copy link

Remaining todos:

Add a section in https://vuejs.org/guide/best-practices/security.html about trusted types compatibility, we can take https://angular.io/guide/security#enforcing-trusted-types as an example.

Am I correct in thinking this also could have a follow up to allow TrustedHTML objects to be passed to v-html? Or is that automatically handled?

@haoqunjiang
Copy link
Member Author

Am I correct in thinking this also could have a follow up to allow TrustedHTML objects to be passed to v-html? Or is that automatically handled?

This is automatically handled as this test case shows: https://github.com/vuejs/core/pull/10844/files#diff-5329e45cfbe289d331762479cc76fe38b6657526dc2a9e2cd6f4729baa6b3e44R81-R102

@yyx990803
Copy link
Member

Since this is a feat I changed the target branch to minor.

This isn't a complete fix because `innerHTML` assignment also occurs
in `insertStaticContent`. But at least this makes the hello-world app
work with trusted types enabled.

I'm still figuring out how to add test cases for trusted types.

Remaining todos:
- [ ] Add test cases for trusted types.
- [ ] Fix `insertStaticContent` to be compatible with trusted types,
we may need a `vue` policy for that case.
- [ ] Add a note in the docs about trusted types compatibility.
- [ ] Allow trusted values to be passed to `v-html` and other props,
this ultimately fixes vuejs/rfcs#614
…p mounting

Because `replaceChildren` isn't supported in all browsers, we still need
to use `innerHTML` to clear the container before mounting the app.

I left the compat mode implementation in `runtime-core` as is because it
doesn't feel right to use a DOM-only API in `runtime-core`.
It seems that the only source of the `content` is the output of
`stringifyStatic` in `compiler-dom`. I guess no additional check is
needed here.
Setting `textContent` to empty string is the equivalent of setting
`innerHTML` to empty string, but it doesn't trigger trusted types policy
violation.

Though maybe not as clear and performant as `trustedTypes.emptyHTML`,
the code is more succint considering that we don't need to check for the
existence of `trustedTypes` before using it.
@yyx990803
Copy link
Member

After looking at the implementation, the additional code isn't that heavy and I think it's better to always support trusted types without having to explicitly turn it on for production. This also avoids having to update relevant docs / plugins to support the new flag.

@yyx990803 yyx990803 merged commit 6d4eb94 into vuejs:minor Aug 2, 2024
7 checks passed
@yyx990803 yyx990803 changed the title feat(runtime-dom): a new __VUE_PROD_TRUSTED_TYPES__ flag for trusted types compatibility feat(runtime-dom): trusted types compatibility Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR requires more reviews version: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants