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

Viewer must be trusted for ssr templates to be supported #25520

Closed
wants to merge 3 commits into from

Conversation

samouri
Copy link
Member

@samouri samouri commented Nov 8, 2019

summary
Viewer needs to be trusted for us to allow rendering templates server-side. This means that there are now three requirements to opt-in to ssr templating: doc-level opt-in, has capability, and being a trusted viewer.

testing done

  • new unit tests added for isSupported
  • verify this doesn't break mail.ru / outlook

Note: I recommend viewing this PR without whitespace.

Fixes b/142607068.

src/ssr-template-helper.js Outdated Show resolved Hide resolved
@samouri
Copy link
Member Author

samouri commented Nov 12, 2019

I just found an issue with this I need to resolve before this can go through. I don't think allowConsoleError currently integrates well when the passed function is async (returns a promise).

Edit: this was also discussed here #15609. Solution seems to be to use expectAsyncConsoleError

@samouri
Copy link
Member Author

samouri commented Nov 12, 2019

In this presubmit logs:

[19:24:10] Found forbidden: "isTrustedViewer" in src/ssr-template-helper.js:61:25
[19:24:10] Usage of this API requires dedicated review due to being privacy sensitive. Please file an issue asking for permission to use if you have not yet done so.

Should I still file a separate issue?

@dreamofabear
Copy link

Nah, you can add it directly to the source whitelist in presubmit-checks.js.

@samouri
Copy link
Member Author

samouri commented Nov 13, 2019

I was able to pretty easily confirm that this doesn't break gmail by using charles proxy. I'm afraid this isn't as simple for email clients like mail.ru since they don't serve from the amp domain.

msg
);
this.ssrTemplateHelper_.isSupported().then(supported => {
userAssert(
Copy link
Contributor

Choose a reason for hiding this comment

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

This has changed from a sync assert (a throw) to a possibly rejected promise. You need to wait on the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just spotted this as well. When I wrote this, I didn't know that userAssert throws. Working on it now 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

meh, my solution stinks. I'll take another look tomorrow to see if I can make it cleaner.

@samouri
Copy link
Member Author

samouri commented Nov 14, 2019

The fix for amp-form is significantly more complex than the rest. I'll split it into its own PR.

@samouri
Copy link
Member Author

samouri commented Nov 18, 2019

Closing this in favor of #25638

@samouri samouri closed this Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants