fix: hardcoded '/' in normalized options.fs, fixes #412, #408

Changes including:
- will not call fs.resolve when normalizing `fs`
- removed hardcoded `/`
- mandatory `fs.dirname` for relative reference
- mandatory `fs.sep`, defaults to '/'

docs/themes/navy/layout/partial/all-contributors.swig

@@ -48,6 +48,8 @@
     <td align="center"><a href="https://n1ru4l.cloud/"><img src="https://avatars.githubusercontent.com/u/14338007?v=4?s=100" width="100px;" alt=""/></a></td>
     <td align="center"><a href="https://github.com/mattvague"><img src="https://avatars.githubusercontent.com/u/64985?v=4?s=100" width="100px;" alt=""/></a></td>
     <td align="center"><a href="https://github.com/bglw"><img src="https://avatars.githubusercontent.com/u/40188355?v=4?s=100" width="100px;" alt=""/></a></td>
+    <td align="center"><a href="https://about.me/jasonkurian"><img src="https://avatars.githubusercontent.com/u/2642545?v=4?s=100" width="100px;" alt=""/></a></td>
+    <td align="center"><a href="https://github.com/dphm"><img src="https://avatars.githubusercontent.com/u/1707217?v=4?s=100" width="100px;" alt=""/></a></td>
   </tr>
 </table>
 

package.json

@@ -14,6 +14,7 @@
     "lint": "eslint \"**/*.ts\" .",
     "check": "npm test && npm run lint",
     "test": "nyc mocha \"test/**/*.ts\"",
+    "test:e2e": "mocha \"test/e2e/**/*.ts\"",
     "perf": "cd benchmark && npm ci && npm start",
     "perf:diff": "bin/perf-diff.sh",
     "perf:engines": "cd benchmark && npm run engines",

src/builtin/filters/string.ts

@@ -7,12 +7,12 @@ import { stringify } from '../../util/underscore'
 import { assert } from '../../util/assert'
 
 export function append (v: string, arg: string) {
-  assert(arguments.length === 2, () => 'append expect 2 arguments')
+  assert(arguments.length === 2, 'append expect 2 arguments')
   return stringify(v) + stringify(arg)
 }
 
 export function prepend (v: string, arg: string) {
-  assert(arguments.length === 2, () => 'prepend expect 2 arguments')
+  assert(arguments.length === 2, 'prepend expect 2 arguments')
   return stringify(arg) + stringify(v)
 }
 

src/fs/browser.ts

@@ -60,3 +60,9 @@ export async function exists (filepath: string) {
 export function existsSync (filepath: string) {
   return true
 }
+
+export function dirname (filepath: string) {
+  return domResolve(filepath, '.')
+}
+
+export const sep = '/'

src/fs/fs.ts

@@ -1,8 +1,20 @@
+import { LoaderOptions } from './loader'
+
 export interface FS {
+  /** check if a file exists asynchronously */
   exists: (filepath: string) => Promise<boolean>;
-  readFile: (filepath: string) => Promise<string>;
+  /** check if a file exists synchronously */
   existsSync: (filepath: string) => boolean;
+  /** read a file asynchronously */
+  readFile: (filepath: string) => Promise<string>;
+  /** read a file synchronously */
   readFileSync: (filepath: string) => string;
-  resolve: (root: string, file: string, ext: string) => string;
+  /** resolve a file against directory, for given `ext` option */
+  resolve: (dir: string, file: string, ext: string, options?: LoaderOptions) => string;
+  /** defaults to "/", will be used for "within roots" check */
+  sep?: string;
+  /** dirname for a filepath, used when resolving relative path */
+  dirname?: (file: string) => string;
+  /** fallback file for lookup failure */
   fallback?: (file: string) => string | undefined;
 }

src/fs/loader.ts

@@ -1,6 +1,8 @@
 import { FS } from './fs'
+import { escapeRegex } from '../util/underscore'
+import { assert } from '../util/assert'
 
-interface LoaderOptions {
+export interface LoaderOptions {
   fs: FS;
   extname: string;
   root: string[];
@@ -15,9 +17,13 @@ export enum LookupType {
 }
 export class Loader {
   private options: LoaderOptions
+  private sep: string
+  private rRelativePath: RegExp
 
   constructor (options: LoaderOptions) {
     this.options = options
+    this.sep = this.options.fs.sep || '/'
+    this.rRelativePath = new RegExp(['.' + this.sep, '..' + this.sep].map(prefix => escapeRegex(prefix)).join('|'))
   }
 
   public * lookup (file: string, type: LookupType, sync?: boolean, currentFile?: string) {
@@ -29,20 +35,12 @@ export class Loader {
     throw this.lookupError(file, dirs)
   }
 
-  public shouldLoadRelative (currentFile: string) {
-    return this.options.relativeReference && this.isRelativePath(currentFile)
-  }
-
-  public isRelativePath (path: string) {
-    return path.startsWith('./') || path.startsWith('../')
-  }
-
   public * candidates (file: string, dirs: string[], currentFile?: string, enforceRoot?: boolean) {
     const { fs, extname } = this.options
     if (this.shouldLoadRelative(file) && currentFile) {
-      const referenced = fs.resolve(this.dirname(currentFile), file, extname)
+      const referenced = fs.resolve(this.dirname(currentFile), file, extname, this.options)
       for (const dir of dirs) {
-        if (!enforceRoot || referenced.startsWith(dir)) {
+        if (!enforceRoot || this.withinDir(referenced, dir)) {
           // the relatively referenced file is within one of root dirs
           yield referenced
           break
@@ -51,7 +49,7 @@ export class Loader {
     }
     for (const dir of dirs) {
       const referenced = fs.resolve(dir, file, extname)
-      if (!enforceRoot || referenced.startsWith(dir)) {
+      if (!enforceRoot || this.withinDir(referenced, dir)) {
         yield referenced
       }
     }
@@ -61,10 +59,19 @@ export class Loader {
     }
   }
 
+  private withinDir (file: string, dir: string) {
+    dir = dir.endsWith(this.sep) ? dir : dir + this.sep
+    return file.startsWith(dir)
+  }
+
+  private shouldLoadRelative (referencedFile: string) {
+    return this.options.relativeReference && this.rRelativePath.test(referencedFile)
+  }
+
   private dirname (path: string) {
-    const segments = path.split('/')
-    segments.pop()
-    return segments.join('/')
+    const fs = this.options.fs
+    assert(fs.dirname, '`fs.dirname` is required for relative reference')
+    return fs.dirname!(path)
   }
 
   private lookupError (file: string, roots: string[]) {

src/fs/node.ts

@@ -1,6 +1,7 @@
 import * as _ from '../util/underscore'
-import { resolve as nodeResolve, extname } from 'path'
+import { resolve as nodeResolve, extname, dirname as nodeDirname } from 'path'
 import { stat, statSync, readFile as nodeReadFile, readFileSync as nodeReadFileSync } from 'fs'
+import { LiquidOptions } from '../liquid-options'
 
 const statAsync = _.promisify(stat)
 const readFileAsync = _.promisify<string, string, string>(nodeReadFile)
@@ -27,7 +28,7 @@ export function existsSync (filepath: string) {
 export function readFileSync (filepath: string) {
   return nodeReadFileSync(filepath, 'utf8')
 }
-export function resolve (root: string, file: string, ext: string) {
+export function resolve (root: string, file: string, ext: string, opts: LiquidOptions) {
   if (!extname(file)) file += ext
   return nodeResolve(root, file)
 }
@@ -36,3 +37,7 @@ export function fallback (file: string) {
     return require.resolve(file)
   } catch (e) {}
 }
+export function dirname (filepath: string) {
+  return nodeDirname(filepath)
+}
+export { sep } from 'path'

src/liquid-options.ts

@@ -169,5 +169,5 @@ export function normalizeDirectoryList (value: any): string[] {
   let list: string[] = []
   if (_.isArray(value)) list = value
   if (_.isString(value)) list = [value]
-  return list.map(str => fs.resolve(str, '.', '')).map(str => str[str.length - 1] !== '/' ? str + '/' : str)
+  return list
 }

src/render/expression.ts

@@ -21,7 +21,7 @@ export class Expression {
     this.postfix = [...toPostfix(tokens)]
   }
   public * evaluate (ctx: Context, lenient: boolean): any {
-    assert(ctx, () => 'unable to evaluate: context not defined')
+    assert(ctx, 'unable to evaluate: context not defined')
     const operands: any[] = []
     for (const token of this.postfix) {
       if (TypeGuards.isOperatorToken(token)) {

src/util/assert.ts

@@ -1,8 +1,10 @@
 import { AssertionError } from './error'
 
-export function assert <T> (predicate: T | null | undefined, message?: () => string) {
+export function assert <T> (predicate: T | null | undefined, message?: string | (() => string)) {
   if (!predicate) {
-    const msg = message ? message() : `expect ${predicate} to be true`
+    const msg = typeof message === 'function'
+      ? message()
+      : (message || `expect ${predicate} to be true`)
     throw new AssertionError(msg)
   }
 }

src/util/underscore.ts

@@ -11,6 +11,10 @@ export function isFunction (value: any): value is Function {
   return typeof value === 'function'
 }
 
+export function escapeRegex (str: string) {
+  return str.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&')
+}
+
 export function promisify<T1, T2> (fn: (arg1: T1, cb: (err: Error | null, result: T2) => void) => void): (arg1: T1) => Promise<T2>;
 export function promisify<T1, T2, T3> (fn: (arg1: T1, arg2: T2, cb: (err: Error | null, result: T3) => void) => void): (arg1: T1, arg2: T2) => Promise<T3>;
 export function promisify (fn: any) {

test/e2e/issues.ts

@@ -114,4 +114,19 @@ describe('Issues', function () {
     const html = await engine.render(tpl, { date: '2021-10-06T15:31:00+08:00' })
     expect(html).to.equal('2021-10-06 17:31 PM +1000')
   })
+  it('#412 Pass root as it is to `resolve`', async () => {
+    const engine = new Liquid({
+      root: '/tmp',
+      fs: {
+        readFileSync: (file: string) => file,
+        async readFile (file: string) { return 'foo' },
+        existsSync (file: string) { return true },
+        async exists (file: string) { return true },
+        resolve: (dir: string, file: string) => dir + '/' + file
+      }
+    })
+    const tpl = engine.parse('{% include "foo.liquid" %}')
+    const html = await engine.renderSync(tpl)
+    expect(html).to.equal('/tmp/foo.liquid')
+  })
 })

test/integration/liquid/fs-option.ts

@@ -6,16 +6,16 @@ use(chaiAsPromised)
 describe('LiquidOptions#fs', function () {
   let engine: Liquid
   const fs = {
-    exists: (x: string) => Promise.resolve(x.match(/^\/root\/files\//)),
-    existsSync: (x: string) => x.match(/^\/root\/files\//),
+    exists: (x: string) => Promise.resolve(!x.match(/not-exist/)),
+    existsSync: (x: string) => !x.match(/not-exist/),
     readFile: (x: string) => Promise.resolve(`content for ${x}`),
     readFileSync: (x: string) => `content for ${x}`,
     fallback: (x: string) => '/root/files/fallback',
-    resolve: (base: string, path: string) => base + path
+    resolve: (base: string, path: string) => base + '/' + path
   }
   beforeEach(function () {
     engine = new Liquid({
-      root: '/root/',
+      root: '/root',
       fs
     } as any)
   })
@@ -25,12 +25,12 @@ describe('LiquidOptions#fs', function () {
   })
 
   it('should support fallback', async function () {
-    const html = await engine.renderFile('notexist/foo')
+    const html = await engine.renderFile('not-exist/foo')
     expect(html).to.equal('content for /root/files/fallback')
   })
 
   it('should support renderSync', function () {
-    const html = engine.renderFileSync('notexist/foo')
+    const html = engine.renderFileSync('not-exist/foo')
     expect(html).to.equal('content for /root/files/fallback')
   })
 
@@ -39,7 +39,7 @@ describe('LiquidOptions#fs', function () {
       root: '/root/',
       fs: { ...fs, fallback: undefined }
     } as any)
-    return expect(engine.renderFile('notexist/foo'))
+    return expect(engine.renderFile('not-exist/foo'))
       .to.be.rejectedWith('Failed to lookup')
   })
 })

test/integration/liquid/liquid.ts

@@ -75,7 +75,7 @@ describe('Liquid', function () {
         extname: '.html'
       })
       return expect(engine.renderFile('/not/exist.html')).to
-        .be.rejectedWith(/Failed to lookup "\/not\/exist.html" in "\/boo\/,\/root\/"/)
+        .be.rejectedWith(/Failed to lookup "\/not\/exist.html" in "\/boo,\/root\/"/)
     })
   })
   describe('#parseFile', function () {
@@ -85,7 +85,7 @@ describe('Liquid', function () {
         extname: '.html'
       })
       return expect(engine.parseFile('/not/exist.html')).to
-        .be.rejectedWith(/Failed to lookup "\/not\/exist.html" in "\/boo\/,\/root\/"/)
+        .be.rejectedWith(/Failed to lookup "\/not\/exist.html" in "\/boo,\/root\/"/)
     })
     it('should fallback to require.resolve in Node.js', async function () {
       const engine = new Liquid({
@@ -144,7 +144,7 @@ describe('Liquid', function () {
         extname: '.html'
       })
       return expect(() => engine.parseFileSync('/not/exist.html'))
-        .to.throw(/Failed to lookup "\/not\/exist.html" in "\/boo\/,\/root\/"/)
+        .to.throw(/Failed to lookup "\/not\/exist.html" in "\/boo,\/root\/"/)
     })
     it('should throw with lookup list when file not exist', function () {
       const engine = new Liquid({
@@ -152,7 +152,7 @@ describe('Liquid', function () {
         extname: '.html'
       })
       return expect(() => engine.parseFileSync('/not/exist.html'))
-        .to.throw(/Failed to lookup "\/not\/exist.html" in "\/boo\/,\/root\/"/)
+        .to.throw(/Failed to lookup "\/not\/exist.html" in "\/boo,\/root\/"/)
     })
   })
   describe('#enderToNodeStream', function () {

test/integration/liquid/root.ts

@@ -1,12 +1,11 @@
 import { normalize } from '../../../src/liquid-options'
 import { expect } from 'chai'
-import { resolve } from 'path'
 
 describe('LiquidOptions#root', function () {
   describe('#normalize ()', function () {
     it('should normalize string typed root array', function () {
       const options = normalize({ root: 'foo' })
-      expect(options.root).to.eql([resolve('foo') + '/'])
+      expect(options.root).to.eql(['foo'])
     })
     it('should normalize null typed root as empty array', function () {
       const options = normalize({ root: null } as any)

test/unit/fs/browser.ts

@@ -6,23 +6,23 @@ import * as chaiAsPromised from 'chai-as-promised'
 use(chaiAsPromised)
 
 describe('fs/browser', function () {
+  if (+(process.version.match(/^v(\d+)/) as RegExpMatchArray)[1] < 8) {
+    console.info('jsdom not supported, skipping template-browser...')
+    return
+  }
+  const JSDOM = require('jsdom').JSDOM
+  beforeEach(function () {
+    const dom = new JSDOM(``, {
+      url: 'https://example.com/foo/bar/',
+      contentType: 'text/html',
+      includeNodeLocations: true
+    });
+    (global as any).document = dom.window.document
+  })
+  afterEach(function () {
+    delete (global as any).document
+  })
   describe('#resolve()', function () {
-    if (+(process.version.match(/^v(\d+)/) as RegExpMatchArray)[1] < 8) {
-      console.info('jsdom not supported, skipping template-browser...')
-      return
-    }
-    const JSDOM = require('jsdom').JSDOM
-    beforeEach(function () {
-      const dom = new JSDOM(``, {
-        url: 'https://example.com/foo/bar/',
-        contentType: 'text/html',
-        includeNodeLocations: true
-      });
-      (global as any).document = dom.window.document
-    })
-    afterEach(function () {
-      delete (global as any).document
-    })
     it('should support relative root', function () {
       expect(fs.resolve('./views/', 'foo', '')).to.equal('https://example.com/foo/bar/views/foo')
     })
@@ -52,6 +52,13 @@ describe('fs/browser', function () {
     })
   })
 
+  describe('#dirname()', () => {
+    it('should return dirname of file', async function () {
+      const val = fs.dirname('https://example.com/views/foo/bar')
+      expect(val).to.equal('https://example.com/views/foo/')
+    })
+  })
+
   describe('#exists()', () => {
     it('should always return true', async function () {
       const val = await fs.exists('/foo/bar')

test/unit/fs/loader.ts

@@ -12,5 +12,15 @@ describe('fs/loader', function () {
       const candidates = [...loader.candidates('./foo/bar', ['/root', '/root/foo'], '/root/current', true)]
       expect(candidates).to.contain('/root/foo/bar')
     })
+    it('should not include out of root candidates', async function () {
+      const loader = new Loader({ relativeReference: true, fs, extname: '' } as any)
+      const candidates = [...loader.candidates('../foo/bar', ['/root'], '/root/current', true)]
+      expect(candidates).to.have.lengthOf(0)
+    })
+    it('should treat root as a terminated path', async function () {
+      const loader = new Loader({ relativeReference: true, fs, extname: '' } as any)
+      const candidates = [...loader.candidates('../root-dir/bar', ['/root'], '/root/current', true)]
+      expect(candidates).to.have.lengthOf(0)
+    })
   })
 })