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

Add (typeof window === 'undefined') check for trustedType #7910

Merged
merged 4 commits into from
Sep 20, 2022

Conversation

licanhua
Copy link
Contributor

@licanhua licanhua commented Sep 19, 2022

Fix #7904

  1. add (typeof window === 'undefined') for everywhere use trustedTypes
  2. make unit test to use node environment instead of jsdom which will not have window object
  3. bump up version with npx lerna version --force-publish --no-push --no-git-tag-version to make min version change
Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Sep 19, 2022

Hi @licanhua. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

@@ -190,7 +190,7 @@ export function truncateText(element: HTMLElement, maxHeight: number, lineHeight
* TextBlock.truncateIfSupported}), but had a bug where it might actually pass through an element
* for which innerHTML yielded actual HTML (since fixed).
*/
const ttDeprecatedPolicy = window.trustedTypes?.createPolicy("adaptivecards#deprecatedExportedFunctionPolicy", {
const ttDeprecatedPolicy = (typeof window === 'undefined') ? undefined : window.trustedTypes?.createPolicy("adaptivecards#deprecatedExportedFunctionPolicy", {
Copy link
Member

Choose a reason for hiding this comment

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

@licanhua , would it be possible to create deprecatedExportedFunctionPolicy before first use? Say host never uses truncate method. Now it still needs to allow it in trusted-types directive so that call .createPolicy() does not fail.

I would prefer if policies were only created when they are needed and used. The same should be also true for other TT policies, I did not check those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's your concern, performance? I think the impact is ignorable.

If you are worried about that "window is undefined" when you import Adaptivecards, then a window object is added. I don't think it's a real use case.

I prefer to keep it simple at the beginning. otherwise, a new function should be added, and
I need to have ttDeprecatedPolicy to be undefined | null, then undefined means not initialized, and null means trusted-types are not supported.

let ttDeprecatedPolicy = undefined | null | PolicyType

function GetPolicy()
{
    if (ttDeprecatedPolicy === undefined) {
       ttDeprecatedPolicy  = null;
       if (typeof window == ....
   } 
}

Copy link
Member

Choose a reason for hiding this comment

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

@licanhua , my concern is that users of AC SDK now need to change trusted-types directive in their CSPs and allow adaptivecards#deprecatedExportedFunctionPolicy even when they are not using it. This goes against best practices to allow only policies that are needed.

Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

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

approved, but please do consider my one comment :)

@licanhua licanhua merged commit dc9a6b4 into release/22.09-js Sep 20, 2022
@licanhua licanhua deleted the canhua/fixWindowIssue branch September 20, 2022 18:17
licanhua added a commit that referenced this pull request Sep 20, 2022
* fix

* bump up version

* fix error on utils

* make test to use node enviroment other than jsdom

(cherry picked from commit dc9a6b4)
licanhua added a commit that referenced this pull request Sep 28, 2022
)

* fix

* bump up version

* fix error on utils

* make test to use node enviroment other than jsdom

(cherry picked from commit dc9a6b4)
michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
…7910) (microsoft#7912)

* fix

* bump up version

* fix error on utils

* make test to use node enviroment other than jsdom

(cherry picked from commit dc9a6b4)
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

3 participants