From 3ab70ec57558ea50e988caf098ed61c21e26bb9d Mon Sep 17 00:00:00 2001 From: Christian Lewis Date: Wed, 20 Jan 2021 01:52:19 -0600 Subject: [PATCH 1/8] feat(headers): add support for consistent * wildcard --- src/utils/headers.js | 145 ++++++++++++++++++++++++++++---------- src/utils/headers.test.js | 81 ++++++++++++++++++++- 2 files changed, 185 insertions(+), 41 deletions(-) diff --git a/src/utils/headers.js b/src/utils/headers.js index b34a1ac6362..c33dc134717 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -3,45 +3,116 @@ const fs = require('fs') const TOKEN_COMMENT = '#' const TOKEN_PATH = '/' -const matchPaths = function (rulePath, targetPath) { - const rulePathParts = rulePath.split('/').filter(Boolean) - const targetPathParts = targetPath.split('/').filter(Boolean) - - if ( - targetPathParts.length === 0 && - (rulePathParts.length === 0 || (rulePathParts.length === 1 && rulePathParts[0] === '*')) - ) { - return true - } - - for (let index = 0; index < rulePathParts.length; index++) { - if (index >= targetPathParts.length) return false - - const rulePart = rulePathParts[index] - const target = targetPathParts[index] - - if (rulePart === '*') return true - - if (rulePart.startsWith(':')) { - if (index === rulePathParts.length - 1) { - return index === targetPathParts.length - 1 - } - if (index === targetPathParts.length - 1) { - return false - } - } else { - return rulePart === target +/** + * @param {Object!} rule + * The pattern to test. + * + * @param {string!} path + * The target path to match against. + * + * @returns {boolean} + * Whether or not the `rulePath` matches the `path`. + */ +const matchPaths = function (rule, path) { + /** + * Break the rule and target paths into pieces. + * + * /a/b/c -> ['a', 'b', 'c'] + */ + const ruleParts = rule.split('/').filter(Boolean) + const pathParts = path.split('/').filter(Boolean) + /** + * Fast path for /*. + */ + const matchAll = ruleParts.length === 1 && ruleParts[0] === '*' + if (matchAll && path) return true + /** + * If either set of path parts is empty (indicating root dir /), they must + * both be empty (/ and /), otherwise there is no match. + */ + const noRuleParts = ruleParts.length === 0 + const noPathParts = pathParts.length === 0 + if (noRuleParts || noPathParts) return noRuleParts && noPathParts + /** + * Otherwise, iterate over ruleParts and pathParts. + */ + for (let index = 0; index < ruleParts.length; index++) { + /** + * Select the corresponding pathPart and rulePart. + */ + const rulePart = ruleParts[index] + const pathPart = pathParts[index] + /** + * Whether or not all ruleParts and pathParts have been iterated over. + */ + const noMoreRuleParts = index >= ruleParts.length - 1 + const noMorePathParts = index >= pathParts.length - 1 + /** + * What kind of placeholder, if any, the current rulePart is. + */ + const ruleIsAsterisk = rulePart === '*' + const ruleIsPlaceholder = rulePart.startsWith(':') + const ruleIsWildcard = ruleIsAsterisk || ruleIsPlaceholder + /** + * For either either wildcard - (*) or (:placeholder) - the entire path is a + * definite match on this wildcard token if: + */ + if (ruleIsWildcard) { + /** + * 1. all ruleParts and pathParts have been iterated over, e.g. + * `/path/to/:placeholder` & `/path/to/*` matched against + * `/path/to/something` (and `/static/*` matched against `/static`) + */ + if (noMorePathParts && noMoreRuleParts) return true + /** + * 2. the rulePart is a (*) wildcard AND it is the final rulePart, e.g. + * `/path/to/*` matched against `/path/to/multiple/subdirs`. + */ + if (noMoreRuleParts && ruleIsAsterisk) return true + } else if (rulePart !== pathPart) { + /** + * If a mismatch is found, the rule does not match. + */ + return false + } else if (noMoreRuleParts) { + /** + * If we have made it through all of the rules without finding a mismatch, + * the rule matches the path. + */ + return true } } - + /** + * If no mismatch was found, the rule matches the target path. + */ return false } -const objectForPath = function (rules, pathname) { - return Object.entries(rules).reduce( - (prev, [rulePath, pathHeaders]) => ({ ...prev, ...(matchPaths(rulePath, pathname) && pathHeaders) }), - {}, - ) +/** + * Get the matching headers for `path` given a set of `rules`. + * + * @param {Object!} rules + * The rules to use for matching. + * + * @param {string!} path + * The path to match against. + * + * @returns {Object} + */ +const objectForPath = function (rules, path) { + /** + * Iterate over the rules and assign the matching headers. + */ + const pathObject = {} + for (const [rule, headers] of Object.entries(rules)) { + /** + * If the rule matches the math, assign the respective headers. + */ + const isMatch = matchPaths(rule, path) + if (isMatch) Object.assign(pathObject, headers) + } + /** Return matched headers. */ + return pathObject } const parseHeadersFile = function (filePath) { @@ -63,11 +134,6 @@ const parseHeadersFile = function (filePath) { if (line.startsWith(TOKEN_COMMENT) || line.length === 0) continue if (line.startsWith(TOKEN_PATH)) { - if (line.includes('*') && line.indexOf('*') !== line.length - 1) { - throw new Error( - `invalid rule (A path rule cannot contain anything after * token) at line: ${index}\n${lines[index]}\n`, - ) - } path = line continue } @@ -97,6 +163,7 @@ const parseHeadersFile = function (filePath) { } module.exports = { + matchPaths, objectForPath, parseHeadersFile, } diff --git a/src/utils/headers.test.js b/src/utils/headers.test.js index 42199203b80..910880bf7d0 100644 --- a/src/utils/headers.test.js +++ b/src/utils/headers.test.js @@ -4,7 +4,7 @@ const test = require('ava') const { createSiteBuilder } = require('../../tests/utils/site-builder') -const { parseHeadersFile, objectForPath } = require('./headers.js') +const { parseHeadersFile, objectForPath, matchPaths } = require('./headers.js') const headers = [ { path: '/', headers: ['X-Frame-Options: SAMEORIGIN'] }, @@ -21,6 +21,20 @@ const headers = [ ], }, { path: '/:placeholder/index.html', headers: ['X-Frame-Options: SAMEORIGIN'] }, + /** + * Do not force * to appear at end of path. + * + * @see https://github.com/netlify/next-on-netlify/issues/151 + * @see https://github.com/netlify/cli/issues/1148 + */ + { + path: '/*/_next/static/chunks/*', + headers: ['cache-control: public', 'cache-control: max-age=31536000', 'cache-control: immutable'], + }, + { + path: '/directory/*/test.html', + headers: ['X-Frame-Options: test'], + }, ] test.before(async (t) => { @@ -38,7 +52,14 @@ test.after(async (t) => { await t.context.builder.cleanupAsync() }) -test('_headers: validate correct parsing', (t) => { +/** + * Pass if we can load the test headers without throwing an error. + */ +test('_headers: syntax validates as expected', (t) => { + parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) +}) + +test('_headers: validate rules', (t) => { const rules = parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) t.deepEqual(rules, { '/': { @@ -55,6 +76,12 @@ test('_headers: validate correct parsing', (t) => { '/:placeholder/index.html': { 'X-Frame-Options': ['SAMEORIGIN'], }, + '/*/_next/static/chunks/*': { + 'cache-control': ['public', 'max-age=31536000', 'immutable'], + }, + '/directory/*/test.html': { + 'X-Frame-Options': ['test'], + }, }) }) @@ -83,4 +110,54 @@ test('_headers: rulesForPath testing', (t) => { 'X-Frame-Options': ['SAMEORIGIN'], 'X-Frame-Thing': ['SAMEORIGIN'], }) + t.deepEqual(objectForPath(rules, '/placeholder/_next/static/chunks/placeholder'), { + 'X-Frame-Thing': ['SAMEORIGIN'], + 'cache-control': ['public', 'max-age=31536000', 'immutable'], + }) + t.deepEqual(objectForPath(rules, '/directory/placeholder/test.html'), { + 'X-Frame-Thing': ['SAMEORIGIN'], + 'X-Frame-Options': ['test'], + }) +}) + +/** + * The bulk of the _headers logic concerns testing whether or not a path matches + * a rule - focus on testing `matchPaths` over `objectForPath` (the latter just + * straightforwardly combines a bunch of objects). + */ +test('_headers: matchPaths matches rules as expected', (t) => { + /** + * Make sure (:placeholder) will NOT match root dir. + */ + t.assert(!matchPaths('/:placeholder', '/')) + /** + * (:placeholder) wildcard tests. + */ + t.assert(matchPaths('/directory/:placeholder', '/directory/test')) + t.assert(matchPaths('/directory/:placeholder/test', '/directory/placeholder/test')) + t.assert(!matchPaths('/directory/:placeholder', '/directory/test/test')) + /** + * (*) wildcard tests. + */ + t.assert(matchPaths('/path/*/dir', '/path/to/dir')) + t.assert(matchPaths('/path/to/*/*/*', '/path/to/one/two/three')) + t.assert(matchPaths('/path/*/to/*/dir', '/path/placeholder/to/placeholder/dir')) + t.assert(!matchPaths('/path/*/to/*/dir', '/path/placeholder/to/placeholder')) + /** + * Trailing (*) wildcard matches recursive subdirs. + */ + t.assert(matchPaths('/path/*', '/path/placeholder/to/placeholder/dir')) + t.assert(matchPaths('/path/to/*', '/path/to/oneDir')) + t.assert(matchPaths('/path/to/*', '/path/to/oneDir/twoDir/threeDir')) + /** + * Trailing (*) wildcard matches parent dir. This is caught automagically by + * index >= pathParts.length && index >= ruleParts.length check. + */ + t.assert(matchPaths('/path/to/dir/*', '/path/to/dir')) + /** + * Mixed (*) and (:placeholder) wildcards. + */ + t.assert(matchPaths('/path/*/to/:placeholder/:placeholder/*', '/path/placeholder/to/placeholder/dir/test')) + t.assert(matchPaths('/path/*/:placeholder', '/path/to/dir')) + t.assert(matchPaths('/path/:placeholder/:placeholder/*', '/path/to/dir/one/two/three')) }) From cc748432f071ea9603b7557117b651e00d99cfb7 Mon Sep 17 00:00:00 2001 From: Christian Lewis Date: Wed, 20 Jan 2021 18:38:35 -0600 Subject: [PATCH 2/8] chore: add new tests, refactor logic --- src/utils/headers.js | 15 ++++++++------- src/utils/headers.test.js | 13 +++++++++++-- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/utils/headers.js b/src/utils/headers.js index c33dc134717..474ebda8214 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -59,16 +59,17 @@ const matchPaths = function (rule, path) { */ if (ruleIsWildcard) { /** - * 1. all ruleParts and pathParts have been iterated over, e.g. - * `/path/to/:placeholder` & `/path/to/*` matched against - * `/path/to/something` (and `/static/*` matched against `/static`) - */ - if (noMorePathParts && noMoreRuleParts) return true - /** - * 2. the rulePart is a (*) wildcard AND it is the final rulePart, e.g. + * 1. the rulePart is a (*) wildcard AND it is the final rulePart, e.g. * `/path/to/*` matched against `/path/to/multiple/subdirs`. */ if (noMoreRuleParts && ruleIsAsterisk) return true + /** + * 2. all ruleParts and pathParts have been iterated over, e.g. + * `/path/to/:placeholder` and `/path/to/*` matched against + * `/path/to/something`, as well as `/static/*` and + * `/static/:placeholder` matched against `/static`) + */ + if (noMorePathParts && noMoreRuleParts) return true } else if (rulePart !== pathPart) { /** * If a mismatch is found, the rule does not match. diff --git a/src/utils/headers.test.js b/src/utils/headers.test.js index 910880bf7d0..80cdd2f757f 100644 --- a/src/utils/headers.test.js +++ b/src/utils/headers.test.js @@ -130,6 +130,10 @@ test('_headers: matchPaths matches rules as expected', (t) => { * Make sure (:placeholder) will NOT match root dir. */ t.assert(!matchPaths('/:placeholder', '/')) + /** + * Make sure (:placeholder) will NOT recursively match subdirs. + */ + t.assert(!matchPaths('/path/to/:placeholder', '/path/two/dir/one/two/three')) /** * (:placeholder) wildcard tests. */ @@ -150,14 +154,19 @@ test('_headers: matchPaths matches rules as expected', (t) => { t.assert(matchPaths('/path/to/*', '/path/to/oneDir')) t.assert(matchPaths('/path/to/*', '/path/to/oneDir/twoDir/threeDir')) /** - * Trailing (*) wildcard matches parent dir. This is caught automagically by - * index >= pathParts.length && index >= ruleParts.length check. + * Trailing wildcards match parent dir. + * + * This is caught automagically by the `index >= pathParts.length - 1 && index + * >= ruleParts.length - 1` checks. */ t.assert(matchPaths('/path/to/dir/*', '/path/to/dir')) + t.assert(matchPaths('/path/to/dir/:placeholder', '/path/to/dir')) + t.assert(matchPaths('/path/to/dir/*/:placeholder', '/path/to/dir/test')) /** * Mixed (*) and (:placeholder) wildcards. */ t.assert(matchPaths('/path/*/to/:placeholder/:placeholder/*', '/path/placeholder/to/placeholder/dir/test')) t.assert(matchPaths('/path/*/:placeholder', '/path/to/dir')) t.assert(matchPaths('/path/:placeholder/:placeholder/*', '/path/to/dir/one/two/three')) + t.assert(matchPaths('/path/to/dir/*/:placeholder/test', '/path/to/dir/asterisk/placeholder/test')) }) From 80e98a14bc3875f4379f571243c13940e8d22653 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Wed, 27 Jan 2021 16:09:40 +0100 Subject: [PATCH 3/8] refactor: cleanup headers parsing code --- src/utils/headers.js | 54 +++++++++++++++++++++------------------ src/utils/headers.test.js | 27 +++++++++++++++++--- 2 files changed, 53 insertions(+), 28 deletions(-) diff --git a/src/utils/headers.js b/src/utils/headers.js index 474ebda8214..c41e1ef8436 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -1,5 +1,7 @@ const fs = require('fs') +const { get } = require('dot-prop') + const TOKEN_COMMENT = '#' const TOKEN_PATH = '/' @@ -117,45 +119,47 @@ const objectForPath = function (rules, path) { } const parseHeadersFile = function (filePath) { - const rules = {} - if (!fs.existsSync(filePath)) return rules + if (!fs.existsSync(filePath)) { + return {} + } if (fs.statSync(filePath).isDirectory()) { console.warn('expected _headers file but found a directory at:', filePath) - return rules + return {} } - const lines = fs.readFileSync(filePath, { encoding: 'utf8' }).split('\n') - if (lines.length === 0) return rules + const lines = fs + .readFileSync(filePath, { encoding: 'utf8' }) + .split('\n') + .map((line, index) => ({ line: line.trim(), index })) + .filter(({ line }) => Boolean(line) && !line.startsWith(TOKEN_COMMENT)) let path - for (let index = 0; index <= lines.length; index++) { - if (!lines[index]) continue - - const line = lines[index].trim() - - if (line.startsWith(TOKEN_COMMENT) || line.length === 0) continue + let rules = {} + for (const { line, index } of lines) { if (line.startsWith(TOKEN_PATH)) { path = line continue } - if (!path) throw new Error('path should come before headers') + if (!path) { + throw new Error('path should come before headers') + } if (line.includes(':')) { - const sepIndex = line.indexOf(':') - if (sepIndex < 1) throw new Error(`invalid header at line: ${index}\n${lines[index]}\n`) - - const key = line.slice(0, sepIndex).trim() - const value = line.slice(sepIndex + 1).trim() + const [key = '', value = ''] = line.split(':', 2) + const trimmedKey = key.trim() + const trimmedValue = value.trim() + if (trimmedKey.length === 0 || trimmedValue.length === 0) { + throw new Error(`invalid header at line: ${index}\n${line}\n`) + } - if (Object.prototype.hasOwnProperty.call(rules, path)) { - if (Object.prototype.hasOwnProperty.call(rules[path], key)) { - rules[path][key].push(value) - } else { - rules[path][key] = [value] - } - } else { - rules[path] = { [key]: [value] } + const currentHeaders = get(rules, `${path}.${trimmedKey}`) || [] + rules = { + ...rules, + [path]: { + ...rules[path], + [trimmedKey]: [...currentHeaders, trimmedValue], + }, } } } diff --git a/src/utils/headers.test.js b/src/utils/headers.test.js index 80cdd2f757f..06ca1db812d 100644 --- a/src/utils/headers.test.js +++ b/src/utils/headers.test.js @@ -39,9 +39,20 @@ const headers = [ test.before(async (t) => { const builder = createSiteBuilder({ siteName: 'site-for-detecting-server' }) - builder.withHeadersFile({ - headers, - }) + builder + .withHeadersFile({ + headers, + }) + .withContentFile({ + path: '_invalid_headers', + content: ` +/ + # This is valid + X-Frame-Options: SAMEORIGIN + # This is not valid + X-Frame-Thing: +`, + }) await builder.buildAsync() @@ -59,6 +70,12 @@ test('_headers: syntax validates as expected', (t) => { parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) }) +test('_headers: throws on invalid syntax', (t) => { + t.throws(() => parseHeadersFile(path.resolve(t.context.builder.directory, '_invalid_headers')), { + message: /invalid header at line: 5/, + }) +}) + test('_headers: validate rules', (t) => { const rules = parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) t.deepEqual(rules, { @@ -85,6 +102,10 @@ test('_headers: validate rules', (t) => { }) }) +test('_headers: throws ', (t) => { + parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) +}) + test('_headers: rulesForPath testing', (t) => { const rules = parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) t.deepEqual(objectForPath(rules, '/'), { From 020a7e5616509eb9b86fc6f5aba06e3029c663bd Mon Sep 17 00:00:00 2001 From: erezrokah Date: Wed, 27 Jan 2021 16:52:23 +0100 Subject: [PATCH 4/8] refactor: simplify objectForPath --- src/utils/headers.js | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/utils/headers.js b/src/utils/headers.js index c41e1ef8436..ab5f639e67c 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -94,7 +94,7 @@ const matchPaths = function (rule, path) { /** * Get the matching headers for `path` given a set of `rules`. * - * @param {Object!} rules + * @param {Object>!} rules * The rules to use for matching. * * @param {string!} path @@ -103,18 +103,11 @@ const matchPaths = function (rule, path) { * @returns {Object} */ const objectForPath = function (rules, path) { - /** - * Iterate over the rules and assign the matching headers. - */ - const pathObject = {} - for (const [rule, headers] of Object.entries(rules)) { - /** - * If the rule matches the math, assign the respective headers. - */ - const isMatch = matchPaths(rule, path) - if (isMatch) Object.assign(pathObject, headers) - } - /** Return matched headers. */ + const matchingHeaders = Object.entries(rules) + .filter(([rule]) => matchPaths(rule, path)) + .map(([, headers]) => headers) + + const pathObject = Object.assign({}, ...matchingHeaders) return pathObject } From 53dd5145e1473bc713ccc98e0e5fab50b898a7c3 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Wed, 27 Jan 2021 17:15:09 +0100 Subject: [PATCH 5/8] refactor: simplify parseHeadersFile --- src/utils/headers.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/utils/headers.js b/src/utils/headers.js index ab5f639e67c..c9a05e14019 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -140,8 +140,7 @@ const parseHeadersFile = function (filePath) { if (line.includes(':')) { const [key = '', value = ''] = line.split(':', 2) - const trimmedKey = key.trim() - const trimmedValue = value.trim() + const [trimmedKey, trimmedValue] = [key.trim(), value.trim()] if (trimmedKey.length === 0 || trimmedValue.length === 0) { throw new Error(`invalid header at line: ${index}\n${line}\n`) } From 36a2ffdd800717306a28393431683977ea50c88d Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 28 Jan 2021 13:33:50 +0100 Subject: [PATCH 6/8] refactor: use regex instead of string matching --- src/utils/headers.js | 123 ++++++++++++-------------------------- src/utils/headers.test.js | 80 ++++++++++++++----------- 2 files changed, 81 insertions(+), 122 deletions(-) diff --git a/src/utils/headers.js b/src/utils/headers.js index c9a05e14019..ada06129955 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -1,94 +1,45 @@ const fs = require('fs') const { get } = require('dot-prop') +const escapeRegExp = require('lodash/escapeRegExp') +const trimEnd = require('lodash/trimEnd') const TOKEN_COMMENT = '#' const TOKEN_PATH = '/' -/** - * @param {Object!} rule - * The pattern to test. - * - * @param {string!} path - * The target path to match against. - * - * @returns {boolean} - * Whether or not the `rulePath` matches the `path`. - */ -const matchPaths = function (rule, path) { - /** - * Break the rule and target paths into pieces. - * - * /a/b/c -> ['a', 'b', 'c'] - */ +const getRulePattern = (rule) => { const ruleParts = rule.split('/').filter(Boolean) - const pathParts = path.split('/').filter(Boolean) - /** - * Fast path for /*. - */ - const matchAll = ruleParts.length === 1 && ruleParts[0] === '*' - if (matchAll && path) return true - /** - * If either set of path parts is empty (indicating root dir /), they must - * both be empty (/ and /), otherwise there is no match. - */ - const noRuleParts = ruleParts.length === 0 - const noPathParts = pathParts.length === 0 - if (noRuleParts || noPathParts) return noRuleParts && noPathParts - /** - * Otherwise, iterate over ruleParts and pathParts. - */ - for (let index = 0; index < ruleParts.length; index++) { - /** - * Select the corresponding pathPart and rulePart. - */ - const rulePart = ruleParts[index] - const pathPart = pathParts[index] - /** - * Whether or not all ruleParts and pathParts have been iterated over. - */ - const noMoreRuleParts = index >= ruleParts.length - 1 - const noMorePathParts = index >= pathParts.length - 1 - /** - * What kind of placeholder, if any, the current rulePart is. - */ - const ruleIsAsterisk = rulePart === '*' - const ruleIsPlaceholder = rulePart.startsWith(':') - const ruleIsWildcard = ruleIsAsterisk || ruleIsPlaceholder - /** - * For either either wildcard - (*) or (:placeholder) - the entire path is a - * definite match on this wildcard token if: - */ - if (ruleIsWildcard) { - /** - * 1. the rulePart is a (*) wildcard AND it is the final rulePart, e.g. - * `/path/to/*` matched against `/path/to/multiple/subdirs`. - */ - if (noMoreRuleParts && ruleIsAsterisk) return true - /** - * 2. all ruleParts and pathParts have been iterated over, e.g. - * `/path/to/:placeholder` and `/path/to/*` matched against - * `/path/to/something`, as well as `/static/*` and - * `/static/:placeholder` matched against `/static`) - */ - if (noMorePathParts && noMoreRuleParts) return true - } else if (rulePart !== pathPart) { - /** - * If a mismatch is found, the rule does not match. - */ - return false - } else if (noMoreRuleParts) { - /** - * If we have made it through all of the rules without finding a mismatch, - * the rule matches the path. - */ - return true - } + if (ruleParts.length === 0) { + return `^/$` } - /** - * If no mismatch was found, the rule matches the target path. - */ - return false + + let pattern = '^' + + ruleParts.forEach((part) => { + if (part.startsWith(':')) { + pattern += '/([^/]+)/?' + } else if (part === '*') { + if (pattern === '^') { + pattern += '/?(.*)/?' + } else { + pattern = trimEnd(pattern, '/?') + pattern += '(?:|/|/(.*)/?)' + } + } else if (part.includes('*')) { + pattern += `/${part.replace(/\*/g, '(.*)')}` + } else if (part.trim() !== '') { + pattern += `/${escapeRegExp(part)}/?` + } + }) + + pattern += '$' + + return pattern +} + +const matchesPath = (rule, path) => { + const pattern = getRulePattern(rule) + return new RegExp(pattern, 'i').test(path) } /** @@ -102,9 +53,9 @@ const matchPaths = function (rule, path) { * * @returns {Object} */ -const objectForPath = function (rules, path) { +const headersForPath = function (rules, path) { const matchingHeaders = Object.entries(rules) - .filter(([rule]) => matchPaths(rule, path)) + .filter(([rule]) => matchesPath(rule, path)) .map(([, headers]) => headers) const pathObject = Object.assign({}, ...matchingHeaders) @@ -160,7 +111,7 @@ const parseHeadersFile = function (filePath) { } module.exports = { - matchPaths, - objectForPath, + matchesPath, + headersForPath, parseHeadersFile, } diff --git a/src/utils/headers.test.js b/src/utils/headers.test.js index 06ca1db812d..59329808e57 100644 --- a/src/utils/headers.test.js +++ b/src/utils/headers.test.js @@ -4,7 +4,7 @@ const test = require('ava') const { createSiteBuilder } = require('../../tests/utils/site-builder') -const { parseHeadersFile, objectForPath, matchPaths } = require('./headers.js') +const { parseHeadersFile, headersForPath, matchesPath } = require('./headers.js') const headers = [ { path: '/', headers: ['X-Frame-Options: SAMEORIGIN'] }, @@ -102,40 +102,36 @@ test('_headers: validate rules', (t) => { }) }) -test('_headers: throws ', (t) => { - parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) -}) - -test('_headers: rulesForPath testing', (t) => { +test('_headers: headersForPath testing', (t) => { const rules = parseHeadersFile(path.resolve(t.context.builder.directory, '_headers')) - t.deepEqual(objectForPath(rules, '/'), { + t.deepEqual(headersForPath(rules, '/'), { 'X-Frame-Options': ['SAMEORIGIN'], 'X-Frame-Thing': ['SAMEORIGIN'], }) - t.deepEqual(objectForPath(rules, '/placeholder'), { + t.deepEqual(headersForPath(rules, '/placeholder'), { 'X-Frame-Thing': ['SAMEORIGIN'], }) - t.deepEqual(objectForPath(rules, '/static-path/placeholder'), { + t.deepEqual(headersForPath(rules, '/static-path/placeholder'), { 'X-Frame-Thing': ['SAMEORIGIN'], 'X-Frame-Options': ['DENY'], 'X-XSS-Protection': ['1; mode=block'], 'cache-control': ['max-age=0', 'no-cache', 'no-store', 'must-revalidate'], }) - t.deepEqual(objectForPath(rules, '/static-path'), { + t.deepEqual(headersForPath(rules, '/static-path'), { 'X-Frame-Thing': ['SAMEORIGIN'], 'X-Frame-Options': ['DENY'], 'X-XSS-Protection': ['1; mode=block'], 'cache-control': ['max-age=0', 'no-cache', 'no-store', 'must-revalidate'], }) - t.deepEqual(objectForPath(rules, '/placeholder/index.html'), { + t.deepEqual(headersForPath(rules, '/placeholder/index.html'), { 'X-Frame-Options': ['SAMEORIGIN'], 'X-Frame-Thing': ['SAMEORIGIN'], }) - t.deepEqual(objectForPath(rules, '/placeholder/_next/static/chunks/placeholder'), { + t.deepEqual(headersForPath(rules, '/placeholder/_next/static/chunks/placeholder'), { 'X-Frame-Thing': ['SAMEORIGIN'], 'cache-control': ['public', 'max-age=31536000', 'immutable'], }) - t.deepEqual(objectForPath(rules, '/directory/placeholder/test.html'), { + t.deepEqual(headersForPath(rules, '/directory/placeholder/test.html'), { 'X-Frame-Thing': ['SAMEORIGIN'], 'X-Frame-Options': ['test'], }) @@ -143,51 +139,63 @@ test('_headers: rulesForPath testing', (t) => { /** * The bulk of the _headers logic concerns testing whether or not a path matches - * a rule - focus on testing `matchPaths` over `objectForPath` (the latter just + * a rule - focus on testing `matchesPath` over `headersForPath` (the latter just * straightforwardly combines a bunch of objects). */ -test('_headers: matchPaths matches rules as expected', (t) => { +test('_headers: matchesPath matches rules as expected', (t) => { + t.assert(matchesPath('/', '/')) + t.assert(matchesPath('/*', '/')) + /** * Make sure (:placeholder) will NOT match root dir. */ - t.assert(!matchPaths('/:placeholder', '/')) + t.assert(!matchesPath('/:placeholder', '/')) + /** * Make sure (:placeholder) will NOT recursively match subdirs. */ - t.assert(!matchPaths('/path/to/:placeholder', '/path/two/dir/one/two/three')) + t.assert(!matchesPath('/path/to/:placeholder', '/path/two/dir/one/two/three')) + /** * (:placeholder) wildcard tests. */ - t.assert(matchPaths('/directory/:placeholder', '/directory/test')) - t.assert(matchPaths('/directory/:placeholder/test', '/directory/placeholder/test')) - t.assert(!matchPaths('/directory/:placeholder', '/directory/test/test')) + t.assert(matchesPath('/directory/:placeholder', '/directory/test')) + t.assert(matchesPath('/directory/:placeholder/test', '/directory/placeholder/test')) + t.assert(!matchesPath('/directory/:placeholder', '/directory/test/test')) + t.assert(!matchesPath('/path/to/dir/:placeholder', '/path/to/dir')) + t.assert(matchesPath('/path/to/dir/:placeholder', '/path/to/dir/placeholder')) + /** * (*) wildcard tests. */ - t.assert(matchPaths('/path/*/dir', '/path/to/dir')) - t.assert(matchPaths('/path/to/*/*/*', '/path/to/one/two/three')) - t.assert(matchPaths('/path/*/to/*/dir', '/path/placeholder/to/placeholder/dir')) - t.assert(!matchPaths('/path/*/to/*/dir', '/path/placeholder/to/placeholder')) + t.assert(matchesPath('/path/*/dir', '/path/to/dir')) + t.assert(matchesPath('/path/to/*/*/*', '/path/to/one/two/three')) + t.assert(matchesPath('/path/*/to/*/dir', '/path/placeholder/to/placeholder/dir')) + t.assert(!matchesPath('/path/*/to/*/dir', '/path/placeholder/to/placeholder')) + t.assert(matchesPath('/path/to/dir/*', '/path/to/dir')) + t.assert(!matchesPath('/*test', '/')) + t.assert(matchesPath('/*test', '/test')) + t.assert(matchesPath('/*test', '/otherTest')) + /** * Trailing (*) wildcard matches recursive subdirs. */ - t.assert(matchPaths('/path/*', '/path/placeholder/to/placeholder/dir')) - t.assert(matchPaths('/path/to/*', '/path/to/oneDir')) - t.assert(matchPaths('/path/to/*', '/path/to/oneDir/twoDir/threeDir')) + t.assert(matchesPath('/path/*', '/path/placeholder/to/placeholder/dir')) + t.assert(matchesPath('/path/to/*', '/path/to/oneDir')) + t.assert(matchesPath('/path/to/*', '/path/to/oneDir/twoDir/threeDir')) + /** * Trailing wildcards match parent dir. * - * This is caught automagically by the `index >= pathParts.length - 1 && index - * >= ruleParts.length - 1` checks. */ - t.assert(matchPaths('/path/to/dir/*', '/path/to/dir')) - t.assert(matchPaths('/path/to/dir/:placeholder', '/path/to/dir')) - t.assert(matchPaths('/path/to/dir/*/:placeholder', '/path/to/dir/test')) + t.assert(matchesPath('/path/to/dir/*', '/path/to/dir')) + t.assert(matchesPath('/path/to/dir/*/:placeholder', '/path/to/dir/test')) + /** * Mixed (*) and (:placeholder) wildcards. */ - t.assert(matchPaths('/path/*/to/:placeholder/:placeholder/*', '/path/placeholder/to/placeholder/dir/test')) - t.assert(matchPaths('/path/*/:placeholder', '/path/to/dir')) - t.assert(matchPaths('/path/:placeholder/:placeholder/*', '/path/to/dir/one/two/three')) - t.assert(matchPaths('/path/to/dir/*/:placeholder/test', '/path/to/dir/asterisk/placeholder/test')) + t.assert(matchesPath('/path/*/to/:placeholder/:placeholder/*', '/path/placeholder/to/placeholder/dir/test')) + t.assert(matchesPath('/path/*/:placeholder', '/path/to/dir')) + t.assert(matchesPath('/path/:placeholder/:placeholder/*', '/path/to/dir/one/two/three')) + t.assert(matchesPath('/path/to/dir/*/:placeholder/test', '/path/to/dir/asterisk/placeholder/test')) }) From b9c85f104c81a8cb665f4cf6744c271185a3083d Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 28 Jan 2021 13:45:49 +0100 Subject: [PATCH 7/8] fix: use new import name --- src/utils/proxy.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/proxy.js b/src/utils/proxy.js index ec663d9742e..4c8ce9f7a21 100644 --- a/src/utils/proxy.js +++ b/src/utils/proxy.js @@ -17,7 +17,7 @@ const toReadableStream = require('to-readable-stream') const { readFileAsync, fileExistsAsync, isFileAsync } = require('../lib/fs.js') const { createStreamPromise } = require('./create-stream-promise') -const { parseHeadersFile, objectForPath } = require('./headers') +const { parseHeadersFile, headersForPath } = require('./headers') const { NETLIFYDEVLOG, NETLIFYDEVWARN } = require('./logo') const { createRewriter } = require('./rules-proxy') const { onChanges } = require('./rules-proxy') @@ -291,7 +291,7 @@ const initializeProxy = function (port, distDir, projectDir) { } } const requestURL = new URL(req.url, `http://${req.headers.host || 'localhost'}`) - const pathHeaderRules = objectForPath(headerRules, requestURL.pathname) + const pathHeaderRules = headersForPath(headerRules, requestURL.pathname) if (!isEmpty(pathHeaderRules)) { Object.entries(pathHeaderRules).forEach(([key, val]) => { res.setHeader(key, val) From 6104d95ab429fc1e1880f328c18b657c5ab4809b Mon Sep 17 00:00:00 2001 From: erezrokah Date: Fri, 29 Jan 2021 11:26:33 +0100 Subject: [PATCH 8/8] refactor: add comments --- src/utils/headers.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/utils/headers.js b/src/utils/headers.js index ada06129955..fae43b1ae43 100644 --- a/src/utils/headers.js +++ b/src/utils/headers.js @@ -7,6 +7,7 @@ const trimEnd = require('lodash/trimEnd') const TOKEN_COMMENT = '#' const TOKEN_PATH = '/' +// Our production logic uses regex too const getRulePattern = (rule) => { const ruleParts = rule.split('/').filter(Boolean) if (ruleParts.length === 0) { @@ -17,8 +18,10 @@ const getRulePattern = (rule) => { ruleParts.forEach((part) => { if (part.startsWith(':')) { + // :placeholder wildcard (e.g. /segment/:placeholder/test) - match everything up to a / pattern += '/([^/]+)/?' } else if (part === '*') { + // standalone asterisk wildcard (e.g. /segment/*) - match everything if (pattern === '^') { pattern += '/?(.*)/?' } else { @@ -26,8 +29,10 @@ const getRulePattern = (rule) => { pattern += '(?:|/|/(.*)/?)' } } else if (part.includes('*')) { + // non standalone asterisk wildcard (e.g. /segment/hello*world/test) pattern += `/${part.replace(/\*/g, '(.*)')}` } else if (part.trim() !== '') { + // not a wildcard pattern += `/${escapeRegExp(part)}/?` } })