Skip to content

Commit 9b5bae2

Browse files
rafa-avila-bcrafa-avila-bc
authored
[STRF-10096] - storefront - investigate google fonts css uri encoding issue (#197)
1 parent 0a0d714 commit 9b5bae2

9 files changed

Lines changed: 111 additions & 48 deletions

File tree

‎helpers/cdn.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ const factory = globals => {
77
const cdn = cdnify(globals);
88

99
return function (path) {
10-
const fullPath = cdn(path);
10+
let fullPath = cdn(path);
1111

1212
const options = arguments[arguments.length - 1];
1313
if (options.hash.resourceHint) {
14-
addResourceHint(
14+
fullPath = addResourceHint(
1515
globals,
1616
fullPath,
1717
options.hash.resourceHint

‎helpers/earlyHint.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,13 @@ const factory = globals => {
88

99
const cors = options.hash.cors;
1010

11-
addResourceHint(
11+
return addResourceHint(
1212
globals,
1313
path,
1414
state,
1515
undefined,
1616
cors
1717
);
18-
19-
return path;
2018
}
2119
};
2220

‎helpers/lib/fonts.js‎

Lines changed: 41 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
'use strict';
22

33
const _ = require('lodash');
4+
const URL = require('url')
45

5-
const {resourceHintAllowedTypes, addResourceHint} = require('../lib/resourceHints');
6+
const { resourceHintAllowedTypes, addResourceHint } = require('../lib/resourceHints');
67

78
const fontProviders = {
89
'Google': {
@@ -19,7 +20,7 @@ const fontProviders = {
1920
*
2021
* @returns {string}
2122
*/
22-
parser: function(fonts) {
23+
parser: function (fonts) {
2324
var collection = [],
2425
familyHash = {};
2526

@@ -53,13 +54,19 @@ const fontProviders = {
5354
return collection;
5455
},
5556

56-
buildLink: function(fonts, fontDisplay) {
57+
buildLink: function (fonts, fontDisplay) {
5758
const displayTypes = ['auto', 'block', 'swap', 'fallback', 'optional'];
5859
fontDisplay = displayTypes.includes(fontDisplay) ? fontDisplay : 'swap';
59-
return `<link href="https://fonts.googleapis.com/css?family=${fonts.join('|')}&display=${fontDisplay}" rel="stylesheet">`;
60+
const uri = `https://fonts.googleapis.com/css?family=${fonts.join('|')}&display=${fontDisplay}`
61+
try {
62+
const url = URL.parse(uri);
63+
return `<link href="${url.format()}" rel="stylesheet">`;
64+
} catch (error) {
65+
throw new Error(`Invalid URL [${uri}]. Check configured fonts.`)
66+
}
6067
},
6168

62-
buildFontLoaderConfig: function(fonts) {
69+
buildFontLoaderConfig: function (fonts) {
6370
function replaceSpaces(font) {
6471
return font.split('+').join(' ');
6572
}
@@ -97,10 +104,10 @@ const fontProviders = {
97104
* @param {Object} options an optional object for additional configuration details
98105
* @returns {Object.<string, Array>|string}
99106
*/
100-
module.exports = function(format, themeSettings, handlebars, options) {
107+
module.exports = function (format, themeSettings, handlebars, options) {
101108

102109
const collectedFonts = {};
103-
_.each(themeSettings, function(value, key) {
110+
_.each(themeSettings, function (value, key) {
104111
//check that -font is on end of string but not start of string
105112
const fontKeySuffix = '-font';
106113
if (!key.endsWith(fontKeySuffix)) {
@@ -122,37 +129,37 @@ module.exports = function(format, themeSettings, handlebars, options) {
122129
});
123130

124131
// Parse font strings based on provider
125-
const parsedFonts = _.mapValues(collectedFonts, function(value, key) {
132+
const parsedFonts = _.mapValues(collectedFonts, function (value, key) {
126133
return fontProviders[key].parser(value);
127134
});
128135

129136
// Format output based on requested format
130-
switch(format) {
131-
case 'linkElements':
137+
switch (format) {
138+
case 'linkElements':
132139

133-
const formattedFonts = _.mapValues(parsedFonts, function(value, key) {
134-
if (options.globals && options.state) {
135-
fontProviders[key].generateResourceHints(options.globals, options.state, value, options.fontDisplay);
136-
}
137-
return fontProviders[key].buildLink(value, options.fontDisplay);
138-
});
139-
return new handlebars.SafeString(_.values(formattedFonts).join(''));
140-
141-
case 'webFontLoaderConfig':
142-
// Build configs
143-
const configs = _.mapValues(parsedFonts, function(value, key) {
144-
return fontProviders[key].buildFontLoaderConfig(value);
145-
});
146-
147-
// Merge them
148-
const fontLoaderConfig = {};
149-
_.each(configs, function(config) {
150-
return Object.assign(fontLoaderConfig, config);
151-
});
152-
return fontLoaderConfig;
153-
154-
case 'providerLists':
155-
default:
156-
return parsedFonts;
140+
const formattedFonts = _.mapValues(parsedFonts, function (value, key) {
141+
if (options.globals && options.state) {
142+
fontProviders[key].generateResourceHints(options.globals, options.state, value, options.fontDisplay);
143+
}
144+
return fontProviders[key].buildLink(value, options.fontDisplay);
145+
});
146+
return new handlebars.SafeString(_.values(formattedFonts).join(''));
147+
148+
case 'webFontLoaderConfig':
149+
// Build configs
150+
const configs = _.mapValues(parsedFonts, function (value, key) {
151+
return fontProviders[key].buildFontLoaderConfig(value);
152+
});
153+
154+
// Merge them
155+
const fontLoaderConfig = {};
156+
_.each(configs, function (config) {
157+
return Object.assign(fontLoaderConfig, config);
158+
});
159+
return fontLoaderConfig;
160+
161+
case 'providerLists':
162+
default:
163+
return parsedFonts;
157164
}
158165
}

‎helpers/lib/resourceHints.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const utils = require("handlebars-utils");
2+
const URL = require('url');
23

34
const resourceHintsLimit = 50;
45

@@ -30,6 +31,11 @@ function addResourceHint(globals, path, state, type, cors) {
3031
throw new Error('Invalid path provided. path should be a non empty string');
3132
}
3233
path = path.trim();
34+
try {
35+
path = URL.parse(path).format();
36+
} catch (error) {
37+
throw new Error(`Invalid value (resource URL) provided: ${path}`)
38+
}
3339

3440
if (!utils.isString(state) || !resourceHintStates.includes(state)) {
3541
throw new Error(`resourceHint attribute received invalid value. Valid values are: ${resourceHintStates}`);
@@ -62,6 +68,8 @@ function addResourceHint(globals, path, state, type, cors) {
6268
hint.cors = cors;
6369

6470
globals.resourceHints.push(hint);
71+
72+
return path;
6573
}
6674

6775
module.exports = {

‎helpers/resourceHints.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ const factory = globals => {
4040
}
4141
}
4242

43-
hosts.forEach(host => {
44-
addResourceHint(
43+
hosts = hosts.map(host => {
44+
return addResourceHint(
4545
globals,
4646
host,
4747
resourceHintAllowedStates.dnsPrefetchResourceHintState,

‎helpers/stylesheet.js‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ const buildCDNHelper = require('./lib/cdnify');
44
const {addResourceHint, resourceHintAllowedTypes} = require('./lib/resourceHints');
55

66
const factory = globals => {
7+
const cdnify = buildCDNHelper(globals);
8+
79
return function (assetPath) {
8-
const cdnify = buildCDNHelper(globals);
910
const siteSettings = globals.getSiteSettings();
1011
const configId = siteSettings.theme_config_id;
1112

@@ -16,10 +17,11 @@ const factory = globals => {
1617
? assetPath.replace(/\.css$/, `-${configId}.css`)
1718
: assetPath;
1819

19-
const url = cdnify(path);
20+
let url = cdnify(path);
21+
2022

2123
if (options.hash.resourceHint) {
22-
addResourceHint(
24+
url = addResourceHint(
2325
globals,
2426
url,
2527
options.hash.resourceHint,

‎spec/helpers/getFontsCollection.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ describe('getFontsCollection', function () {
2121
};
2222
const renderer = buildRenderer({}, themeSettings);
2323
const runTestCases = testRunner({renderer});
24-
const href = "https://fonts.googleapis.com/css?family=Open+Sans:,400italic,700|Karla:700|Lora:400|Volkron:|Droid:400,700|Crimson+Text:400,700&display=swap";
24+
const href = "https://fonts.googleapis.com/css?family=Open+Sans:,400italic,700%7CKarla:700%7CLora:400%7CVolkron:%7CDroid:400,700%7CCrimson+Text:400,700&display=swap";
2525
runTestCases([
2626
{
2727
input: '{{getFontsCollection resourceHint="preload"}}',
@@ -53,7 +53,7 @@ describe('getFontsCollection', function () {
5353
runTestCases([
5454
{
5555
input: '{{getFontsCollection font-display="oreo"}}',
56-
output: '<link href="https://fonts.googleapis.com/css?family=Open+Sans:,400italic,700|Karla:700|Lora:400|Volkron:|Droid:400,700|Crimson+Text:400,700&display=swap" rel="stylesheet">',
56+
output: '<link href="https://fonts.googleapis.com/css?family=Open+Sans:,400italic,700%7CKarla:700%7CLora:400%7CVolkron:%7CDroid:400,700%7CCrimson+Text:400,700&display=swap" rel="stylesheet">',
5757
},
5858
], done);
5959
});
@@ -74,7 +74,7 @@ describe('getFontsCollection', function () {
7474
runTestCases([
7575
{
7676
input: '{{getFontsCollection font-display="fallback"}}',
77-
output: '<link href="https://fonts.googleapis.com/css?family=Open+Sans:,400italic,700|Karla:700|Lora:400|Volkron:|Droid:400,700|Crimson+Text:400,700&display=fallback" rel="stylesheet">',
77+
output: '<link href="https://fonts.googleapis.com/css?family=Open+Sans:,400italic,700%7CKarla:700%7CLora:400%7CVolkron:%7CDroid:400,700%7CCrimson+Text:400,700&display=fallback" rel="stylesheet">',
7878
},
7979
], done);
8080
});

‎spec/helpers/lib/resourceHints.js‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,5 +141,53 @@ describe('resource hints', function () {
141141
expect(f).to.throw();
142142
done();
143143
});
144+
145+
it('should handle URLs with special chars', done => {
146+
const data = [
147+
{
148+
original: "https://bigcommerce.paystackintegrations.com/js/script.js?store_hash=cag1k6vhir&installed_at=Tue May 10 2022 08:57:26 GMT+0000 (Coordinated Universal Time)",
149+
expected: "https://bigcommerce.paystackintegrations.com/js/script.js?store_hash=cag1k6vhir&installed_at=Tue%20May%2010%202022%2008:57:26%20GMT+0000%20(Coordinated%20Universal%20Time)"
150+
},
151+
{
152+
original: `https://instocknotify.blob.core.windows.net/stencil/cfa17df4-72e5-4c23-a4c7-1fa6903bdeb4.js?ts=17443383" type="text/javascript""`,
153+
expected: "https://instocknotify.blob.core.windows.net/stencil/cfa17df4-72e5-4c23-a4c7-1fa6903bdeb4.js?ts=17443383%22%20type=%22text/javascript%22%22"
154+
},
155+
{
156+
original: `https://instocknotify.blob.core.windows.net/stencil/41f9521c-57b6-4920-827b-77ba3477fcb5.js?ts=34911865" type="text/javascript"></script> <!--End InStockNotify Stencil Script -->`,
157+
expected: "https://instocknotify.blob.core.windows.net/stencil/41f9521c-57b6-4920-827b-77ba3477fcb5.js?ts=34911865%22%20type=%22text/javascript%22%3E%3C/script%3E%20%3C!--End%20InStockNotify%20Stencil%20Script%20--%3E"
158+
},
159+
{
160+
original: `https://fonts.googleapis.com/css?family=Karla:400|Montserrat:400,500,700&display=block`,
161+
expected: `https://fonts.googleapis.com/css?family=Karla:400%7CMontserrat:400,500,700&display=block`
162+
},
163+
{
164+
original: '/relative/asset/background.png?why=not',
165+
expected: '/relative/asset/background.png?why=not'
166+
}
167+
];
168+
const globals = {resourceHints: []};
169+
170+
data.forEach(pair => {
171+
const {original, expected} = pair;
172+
173+
const type = 'style';
174+
const state = 'preload';
175+
const cors = 'anonymous';
176+
177+
expect(
178+
expected,
179+
addResourceHint(globals, original, state, type, cors)
180+
);
181+
});
182+
done();
183+
});
184+
185+
it('should throw when invalid URL is passed in', done => {
186+
const globals = {resourceHints: []};
187+
const invalid = '';
188+
const f = () => addResourceHint(globals, invalid, 'style', 'preload', 'no')
189+
expect(f).to.throw();
190+
done();
191+
});
144192
});
145193
});

‎spec/helpers/resourceHints.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ describe('resourceHints', function () {
3030
runTestCases([
3131
{
3232
input: '{{resourceHints}}',
33-
output: '<link rel="dns-prefetch preconnect" href="https://fonts.googleapis.com" crossorigin><link rel="dns-prefetch preconnect" href="https://fonts.gstatic.com" crossorigin>',
33+
output: '<link rel="dns-prefetch preconnect" href="https://fonts.googleapis.com/" crossorigin><link rel="dns-prefetch preconnect" href="https://fonts.gstatic.com/" crossorigin>',
3434
},
3535
], () => {
3636
const hints = renderer.getResourceHints();

0 commit comments

Comments
 (0)