fix: path traversal vulnerability, #851 (#855)

* Fix Path Traversal fallback

* Update loader.ts

Fixed nested

* Update loader.ts

padding fix

* refactor: reuse root enforcing

* docs: update test case and docs

---------

Co-authored-by: MorielHarush <[email protected]>

demo/esm/index.mjs

@@ -8,7 +8,7 @@ const engine = new Liquid({
   // layout files for `{% layout %}`
   layouts: process.cwd() + '/layouts',
   // partial files for `{% include %}` and `{% render %}`
-  partials: process.cwd() + '/partials'
+  partials: [process.cwd() + '/partials', 'node_modules']
 })
 
 const ctx = {

demo/esm/test.sh

@@ -1,3 +1,3 @@
-set -ex
+set -e
 
 npm start | grep 'LiquidJS Demo'

demo/express/test.sh

@@ -1,4 +1,4 @@
-set -x
+set -e
 
 LOG_FILE=$(mktemp)
 npm start > $LOG_FILE 2>&1 &

demo/nodejs/test.sh

@@ -1,3 +1,3 @@
-set -ex
+set -e
 
 npm start | grep 'NodeJS Demo for LiquidJS'

demo/template/test.sh

@@ -1,3 +1,3 @@
-set -ex
+set -e
 
 npm start | grep '\[11:8] {{ todo }}'

demo/typescript/test.sh

@@ -1,3 +1,3 @@
-set -ex
+set -e
 
 npm run build && npm start | grep 'TypeScript Demo for LiquidJS'

demo/webpack/test.sh

@@ -1,4 +1,4 @@
-set -ex
+set -e
 
 npm run build
 npm start | grep 'Webpack Demo for LiquidJS'

docs/source/tutorials/render-file.md

@@ -45,26 +45,20 @@ It can be a string-typed path (see above example), or a list of root directories
 
 ```javascript
 var engine = new Liquid({
-    root: ['views/', 'views/partials/'],
+    root: ['views/'],
+    partials: ['views/partials/'],
+    layouts: ['views/layouts/'],
     extname: '.liquid'
 });
 ```
 
 {% note tip Relative Paths %}Relative paths in <code>root</code> will be resolved against <code>cwd()</code>.{% endnote %}
 
-When `{% raw %}{% render "foo" %}{% endraw %}` is rendered or `liquid.renderFile('foo')` is called, the following files will be looked up and the first existing file will be used:
+- When `parse()`, `render()` functions are called, for example `liquid.renderFile('foo')`, templates under `root` will be looked up.
+- When a partial is requested, for example `{% raw %}{% render "foo" %}{% endraw %}`, templates under `partials` will be looked up.
+- When a layout is requested, for example `{% raw %}{% layout "foo" %}{% endraw %}`, templates under `layouts` will be looked up.
 
-- `cwd()`/views/foo.liquid
+When LiquidJS is used in browser, the paths will be resolved based on current location. Here's a demo for browsers: [demo/browser](https://github.com/harttle/liquidjs/tree/master/demo/browser).
-- `cwd()`/views/partials/foo.liquid
-
-If none of the above files exists, an `ENOENT` error will be thrown. Here's a demo for Node.js: [demo/nodejs](https://github.com/harttle/liquidjs/tree/master/demo/nodejs).
-
-When LiquidJS is used in browser, say current location is <https://example.com/bar/index.html>, only the first `root` will be used and the file to be fetched is:
-
-- <https://example.com/bar/foo.liquid>
-
-If fetch fails, a 404/500 error or network failures for example, an `ENOENT` error will be thrown.
-Here's a demo for browsers: [demo/browser](https://github.com/harttle/liquidjs/tree/master/demo/browser).
 
 ## Abstract File System
 

src/fs/loader.ts

@@ -43,25 +43,26 @@ export class Loader {
 
   public * candidates (file: string, dirs: string[], currentFile?: string, enforceRoot?: boolean) {
     const { fs, extname } = this.options
+    const isAllowed = (filepath: string) => {
+      if (!enforceRoot) return true
+      for (const dir of dirs) {
+        if (this.contains(dir, filepath)) return true
+      }
+      return false
+    }
+
     if (this.shouldLoadRelative(file) && currentFile) {
       const referenced = fs.resolve(this.dirname(currentFile), file, extname)
-      for (const dir of dirs) {
+      if (isAllowed(referenced)) yield referenced
-        if (!enforceRoot || this.contains(dir, referenced)) {
-          // the relatively referenced file is within one of root dirs
-          yield referenced
-          break
-        }
-      }
     }
     for (const dir of dirs) {
       const referenced = fs.resolve(dir, file, extname)
-      if (!enforceRoot || this.contains(dir, referenced)) {
+      if (isAllowed(referenced)) yield referenced
-        yield referenced
-      }
     }
+
     if (fs.fallback !== undefined) {
       const filepath = fs.fallback(file)
-      if (filepath !== undefined) yield filepath
+      if (filepath !== undefined && isAllowed(filepath)) yield filepath
     }
   }
 

test/e2e/issues.spec.ts

@@ -1,4 +1,6 @@
 import { TopLevelToken, TagToken, Tokenizer, Context, Liquid, Drop, toValueSync, LiquidError, IfTag } from '../..'
+import { spawnSync } from 'child_process'
+import { resolve as resolvePath } from 'path'
 const LiquidUMD = require('../../dist/liquid.browser.umd.js').Liquid
 
 describe('Issues', function () {
@@ -173,6 +175,24 @@ describe('Issues', function () {
     const html = await engine.render(tpl, { my_variable: 'foo' })
     expect(html).toBe('CONTENT for /tmp/prefix/foo-bar/suffix')
   })
+  it('should prevent path traversal in dynamic include with restricted root, #851', () => {
+    const projectRoot = resolvePath(__dirname, '../..')
+    const poc = `
+      const { Liquid } = require('./dist/liquid.node.js');
+      const e = new Liquid({ root: ['/tmp'], partials: ['/tmp'], dynamicPartials: true });
+      e.parseAndRender('{% include page %}', { page: '../../../etc/passwd' })
+        .then(() => { console.log('OK'); })
+        .catch(err => { console.error('ERR:' + err.message); process.exit(1); });
+    `
+    const result = spawnSync(
+      process.execPath,
+      ['-e', poc],
+      { cwd: projectRoot, encoding: 'utf8' }
+    )
+
+    expect(result.status).not.toBe(0)
+    expect(result.stderr).toContain('Failed to lookup')
+  })
   it('Implement liquid/echo tags #428', () => {
     const template = `{%- liquid
       for value in array

test/tsconfig.json

@@ -10,8 +10,7 @@
     "allowSyntheticDefaultImports": true,
     "resolveJsonModule": true,
     "downlevelIteration": true,
-    "strict": true,
+    "strict": true
-    "suppressImplicitAnyIndexErrors": true
   },
   "all": true
 }