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

Issue 1242 url preview #1247

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/common/gristUrls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ export const commonUrls = {
formulaSheet: 'https://support.getgrist.com/formula-cheat-sheet',
formulas: 'https://support.getgrist.com/formulas',
forms: 'https://www.getgrist.com/forms/?utm_source=grist-forms&utm_medium=grist-forms&utm_campaign=forms-footer',
openGraphPreviewImage: 'https://grist-static.com/icons/opengraph-preview-image.png',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little concerned about GDPR for European self-hosters opening their instances publicly.
They should have a mechanism to a least offer user deactivation of such url sending data to a third party.
I'm realizing that there is a lot of urls in DINUM and ANCT instances sending IPs data to a third party in GDRP terms.
May be it could be a good Idea to open an issue about that.


gristLabsCustomWidgets: 'https://gristlabs.github.io/grist-widget/',
gristLabsWidgetRepository: 'https://github.com/gristlabs/grist-widget/releases/download/latest/manifest.json',
Expand Down
68 changes: 42 additions & 26 deletions app/server/lib/sendAppPage.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
commonUrls,
Features,
getContactSupportUrl,
getFreeCoachingCallUrl,
Expand All @@ -24,9 +25,22 @@ import * as fse from 'fs-extra';
import * as handlebars from 'handlebars';
import jsesc from 'jsesc';
import * as path from 'path';
import difference = require('lodash/difference');
import difference from 'lodash/difference';

const translate = (req: express.Request, key: string, args?: any) => req.t(`sendAppPage.${key}`, args);
const { escapeExpression } = handlebars.Utils;

/**
* Return the translation given the key, but also ensure that the return value is HTML-escaped
* in order to avoid possible script injection (that we don't need in any case).
*
* @param req
* @param key The key of the translation (which will be prefixed by `sendAppPage`)
* @param args The args to pass to the translation string (optional)
*/
function translateEscaped(req: express.Request, key: string, args?: any) {
Copy link
Member

Choose a reason for hiding this comment

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

This helper is giving me pause. I guess anything it is used to translate won't be picked up by https://github.com/gristlabs/grist-core/blob/main/buildtools/generate_translation_keys.js - not necessarily a problem, since you can add manually - but makes the eventual garbage collection problem harder of figuring out what keys are no longer used as code changes so they can be dropped.

The escaping, the threat model here is that the translations may be tainted, we can't trust them? This does make sense given how our weblate project is run, it is pretty open. Just going to page @berhalak here for some advice on whether there might be a more systematic way to handle this threat.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave this helper for now. I will remove it later and prepare some cleaning mechanism at the build time. Same issue is for client, when someone uses it insecurely (like el.innerHTML = t(....)).
I'm surprised I didn't think of it myself :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@berhalak Right, yeah. Thanks for your feedback, do you want me to create a separate issue for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @fflorent, Yes please. I'll try to do it asap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @berhalak

Reported here as a bug: #1350

const translation = req.t(`sendAppPage.${key}`, args)?.toString();
return translation ? escapeExpression(translation) : translation;
}

export interface ISendAppPageOptions {
path: string; // Ignored if .content is present (set to "" for clarity).
Expand Down Expand Up @@ -174,8 +188,8 @@ export function makeSendAppPage({ server, staticDir, tag, testLogin, baseDomain
).join('\n');
const content = fileContent
.replace("<!-- INSERT WARNING -->", warning)
.replace("<!-- INSERT TITLE -->", getPageTitle(req, config))
.replace("<!-- INSERT META -->", getPageMetadataHtmlSnippet(config))
.replace("<!-- INSERT TITLE -->", getDocName(config) ?? escapeExpression(translateEscaped(req, 'Loading')))
.replace("<!-- INSERT META -->", getPageMetadataHtmlSnippet(req, config))
.replace("<!-- INSERT TITLE SUFFIX -->", getPageTitleSuffix(server.getGristConfig()))
.replace("<!-- INSERT BASE -->", `<base href="${staticBaseUrl}">` + tagManagerSnippet)
.replace("<!-- INSERT LOCALE -->", preloads)
Expand Down Expand Up @@ -264,18 +278,15 @@ function configuredPageTitleSuffix() {
}

/**
* Returns a page title suitable for inserting into an HTML title element.
*
* Currently returns the document name if the page being requested is for a document, or
* a placeholder, "Loading...", that's updated in the client once the page has loaded.
* Returns the doc name.
*
* Note: The string returned is escaped and safe to insert into HTML.
*
*/
function getPageTitle(req: express.Request, config: GristLoadConfig): string {
function getDocName(config: GristLoadConfig): string|null {
const maybeDoc = getDocFromConfig(config);
if (!maybeDoc) { return translate(req, 'Loading') + "..."; }

return handlebars.Utils.escapeExpression(maybeDoc.name);
return maybeDoc && escapeExpression(maybeDoc.name);
}

/**
Expand All @@ -286,25 +297,30 @@ function getPageTitle(req: express.Request, config: GristLoadConfig): string {
*
* Note: The string returned is escaped and safe to insert into HTML.
*/
function getPageMetadataHtmlSnippet(config: GristLoadConfig): string {
function getPageMetadataHtmlSnippet(req: express.Request, config: GristLoadConfig): string {
const metadataElements: string[] = [];
const maybeDoc = getDocFromConfig(config);

const description = maybeDoc?.options?.description;
if (description) {
const content = handlebars.Utils.escapeExpression(description);
metadataElements.push(`<meta name="description" content="${content}">`);
metadataElements.push(`<meta property="og:description" content="${content}">`);
metadataElements.push(`<meta name="twitter:description" content="${content}">`);
}
metadataElements.push('<meta property="og:type" content="website">');
metadataElements.push('<meta name="twitter:card" content="summary_large_image">');

const icon = maybeDoc?.options?.icon;
if (icon) {
const content = handlebars.Utils.escapeExpression(icon);
metadataElements.push(`<meta name="thumbnail" content="${content}">`);
metadataElements.push(`<meta property="og:image" content="${content}">`);
metadataElements.push(`<meta name="twitter:image" content="${content}">`);
}
const description = maybeDoc?.options?.description ?
escapeExpression(maybeDoc.options.description) :
translateEscaped(req, 'og-description');
metadataElements.push(`<meta name="description" content="${description}">`);
metadataElements.push(`<meta property="og:description" content="${description}">`);
metadataElements.push(`<meta name="twitter:description" content="${description}">`);

const image = escapeExpression(maybeDoc?.options?.icon ?? commonUrls.openGraphPreviewImage);
metadataElements.push(`<meta name="thumbnail" content="${image}">`);
metadataElements.push(`<meta property="og:image" content="${image}">`);
metadataElements.push(`<meta name="twitter:image" content="${image}">`);

const maybeDocTitle = getDocName(config);
const title = maybeDocTitle ? maybeDocTitle + getPageTitleSuffix(config) : translateEscaped(req, 'og-title');
// NB: We don't generate the content of the <title> tag here.
metadataElements.push(`<meta property="og:title" content="${title}">`);
metadataElements.push(`<meta name="twitter:title" content="${title}">`);

return metadataElements.join('\n');
}
Expand Down
4 changes: 3 additions & 1 deletion static/locales/en.server.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"sendAppPage": {
"Loading": "Loading"
"Loading": "Loading...",
"og-description": "A modern, open source spreadsheet that goes beyond the grid",
"og-title": "Grist, the evolution of spreadsheets"
},
"oidc": {
"emailNotVerifiedError": "Please verify your email with the identity provider, and log in again."
Expand Down
2 changes: 1 addition & 1 deletion static/locales/fr.server.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"sendAppPage": {
"Loading": "Chargement"
"Loading": "Chargement..."
}
}
17 changes: 17 additions & 0 deletions test/nbrowser/HomeIntro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('HomeIntro', function() {
});

it('should should intro screen for anon', () => testIntroScreen({anon: true, team: false}));
it('should should set meta tags for URL previews', testMetaTags);
it('should not show Other Sites section', testOtherSitesSection);
it('should allow create/import from intro screen', testCreateImport.bind(null, false));
it('should link to examples page from the intro', testExamplesPage);
Expand Down Expand Up @@ -129,6 +130,22 @@ describe('HomeIntro', function() {
}
}

async function testMetaTags() {
const expectedTitle = 'Grist, the evolution of spreadsheets';
assert.equal(await driver.find('meta[name="twitter:title"]').getAttribute('content'), expectedTitle);
assert.equal(await driver.find('meta[property="og:title"]').getAttribute('content'), expectedTitle);

const expectedDescription = 'A modern, open source spreadsheet that goes beyond the grid';
fflorent marked this conversation as resolved.
Show resolved Hide resolved
assert.equal(await driver.find('meta[name="description"]').getAttribute('content'), expectedDescription);
assert.equal(await driver.find('meta[name="twitter:description"]').getAttribute('content'), expectedDescription);
assert.equal(await driver.find('meta[property="og:description"]').getAttribute('content'), expectedDescription);

const gristIconFileName = 'opengraph-preview-image.png';
assert.include(await driver.find('meta[name="thumbnail"]').getAttribute('content'), gristIconFileName);
assert.include(await driver.find('meta[name="twitter:image"]').getAttribute('content'), gristIconFileName);
assert.include(await driver.find('meta[property="og:image"]').getAttribute('content'), gristIconFileName);
}

async function testCreateImport(isLoggedIn: boolean) {
await checkIntroButtons(isLoggedIn);

Expand Down
17 changes: 16 additions & 1 deletion test/nbrowser/NewDocument.ntest.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,22 @@ describe('NewDocument.ntest', function() {
this.timeout(10000);
await gu.actions.createNewDoc('Untitled');
assert.equal(await gu.actions.getDocTitle(), 'Untitled');
assert.equal(await driver.getTitle(), 'Untitled - Grist');

const expectedTitle = 'Untitled - Grist';
assert.equal(await driver.getTitle(), expectedTitle);
assert.equal(await driver.find('meta[name="twitter:title"]').getAttribute('content'), expectedTitle);
assert.equal(await driver.find('meta[property="og:title"]').getAttribute('content'), expectedTitle);

const expectedDescription = 'A modern, open source spreadsheet that goes beyond the grid';
assert.equal(await driver.find('meta[name="description"]').getAttribute('content'), expectedDescription);
assert.equal(await driver.find('meta[name="twitter:description"]').getAttribute('content'), expectedDescription);
assert.equal(await driver.find('meta[property="og:description"]').getAttribute('content'), expectedDescription);

const gristIconFileName = 'opengraph-preview-image.png';
assert.include(await driver.find('meta[name="thumbnail"]').getAttribute('content'), gristIconFileName);
assert.include(await driver.find('meta[name="twitter:image"]').getAttribute('content'), gristIconFileName);
assert.include(await driver.find('meta[property="og:image"]').getAttribute('content'), gristIconFileName);

assert.equal(await $('.active_section .test-viewsection-title').wait().text(), 'TABLE1');
await gu.waitForServer();
});
Expand Down
Loading