fix: use realpath for fs.contains (#867)

* fix: use realpath for fs.contains

* chore: reset file mode changes

Made-with: Cursor

* fix: Windows compat for contains/containsSync and toLiquidAsync arg order

Made-with: Cursor

Author: harttle

.cursor/rules/architecture.mdc

@@ -0,0 +1,17 @@
+---
+description: Architecture overview for liquidjs internals
+alwaysApply: true
+---
+
+## Async/sync duality via generators
+
+All core logic is written once as a `Generator` function (`function *`). Use `yield` where you'd normally `await` a potentially async value.
+
+- `toPromise(generator)` drives it **asynchronously** — awaits yielded promises.
+- `toValueSync(generator)` drives it **synchronously** — passes yielded values through as-is.
+
+Never duplicate logic into separate async and sync methods. A single generator serves both paths.
+
+When wrapping an async+sync function pair (e.g. `contains`/`containsSync`, `exists`/`existsSync`, `readFile`/`readFileSync`), use `toLiquidAsync(asyncFn, syncFn?)` which returns a `LiquidAsync<F>` — one function that picks the sync or async implementation based on a leading `sync: boolean` arg. Then `yield` the result inside a generator to let the driver handle it in both modes.
+
+See `src/util/async.ts`.

.cursor/rules/conventions.mdc

@@ -0,0 +1,6 @@
+---
+description: Project conventions for liquidjs
+alwaysApply: true
+---
+
+- Keep edits minimal: change only what the task requires, match existing style.

docs/source/tutorials/render-file.md

@@ -92,7 +92,7 @@ var engine = new Liquid({
 });
 ```
 
-{% note warn Path Traversal Vulnerability %}The default value of <code>contains()</code> always returns true. That means when specifying an abstract file system, you'll need to provide a proper <code>contains()</code> to avoid expose such vulnerabilities.{% endnote %}
+{% note warn Path Traversal Vulnerability %}The built-in Node <code>fs</code> implements <code>contains()</code> with realpath so templates cannot escape the root via symlinks. The browser bundle omits <code>contains</code> (loader treats paths as allowed). For a custom abstract <code>fs</code>, implement <code>contains</code> unless every resolved path is trusted.{% endnote %}
 
 ## In-memory Template
 

src/fs/fs-impl.spec.ts

@@ -1,5 +1,9 @@
 import * as fs from './fs-impl'
 import * as path from 'path'
+import { mkdtempSync, writeFileSync, symlinkSync, rmSync } from 'fs'
+import { tmpdir } from 'os'
+
+const { join } = path
 
 describe('fs-impl', function () {
   describe('.resolve()', function () {
@@ -50,4 +54,36 @@ describe('fs-impl', function () {
       expect(content).toContain('should read content if exists')
     })
   })
+  describe('.contains()', () => {
+    const canSymlink = process.platform !== 'win32'
+    ;(canSymlink ? it : it.skip)('should return false when path is a symlink to outside root', async () => {
+      const root = mkdtempSync(join(tmpdir(), 'liquid-contains-'))
+      const outside = join(tmpdir(), `secret-${Date.now()}.liquid`)
+      writeFileSync(outside, 'x')
+      const link = join(root, 'link.liquid')
+      symlinkSync(outside, link)
+      try {
+        expect(await fs.contains(root, link)).toBe(false)
+      } finally {
+        rmSync(root, { recursive: true, force: true })
+        rmSync(outside, { force: true })
+      }
+    })
+  })
+  describe('.containsSync()', () => {
+    const canSymlink = process.platform !== 'win32'
+    ;(canSymlink ? it : it.skip)('should return false when path is a symlink to outside root', () => {
+      const root = mkdtempSync(join(tmpdir(), 'liquid-contains-'))
+      const outside = join(tmpdir(), `secret-${Date.now()}.liquid`)
+      writeFileSync(outside, 'x')
+      const link = join(root, 'link.liquid')
+      symlinkSync(outside, link)
+      try {
+        expect(fs.containsSync(root, link)).toBe(false)
+      } finally {
+        rmSync(root, { recursive: true, force: true })
+        rmSync(outside, { force: true })
+      }
+    })
+  })
 })

src/fs/fs-impl.ts

@@ -1,6 +1,6 @@
 import { promisify } from '../util'
 import { sep, resolve as nodeResolve, extname, dirname as nodeDirname } from 'path'
-import { stat, statSync, readFile as nodeReadFile, readFileSync as nodeReadFileSync } from 'fs'
+import { stat, statSync, readFile as nodeReadFile, readFileSync as nodeReadFileSync, realpath, realpathSync } from 'fs'
 import { requireResolve } from './node-require'
 
 type NodeReadFile = (file: string, encoding: string, cb: ((err: Error | null, result: string) => void)) => void
@@ -41,10 +41,27 @@ export function fallback (file: string) {
 export function dirname (filepath: string) {
   return nodeDirname(filepath)
 }
-export function contains (root: string, file: string) {
-  root = nodeResolve(root)
-  root = root.endsWith(sep) ? root : root + sep
-  return file.startsWith(root)
+const realpathAsync = promisify(realpath)
+
+export async function contains (root: string, file: string) {
+  try {
+    const realRoot = await realpathAsync(root)
+    const realFile = await realpathAsync(file)
+    const prefix = realRoot.endsWith(sep) ? realRoot : realRoot + sep
+    return realFile.startsWith(prefix)
+  } catch {
+    return false
+  }
+}
+export function containsSync (root: string, file: string) {
+  try {
+    const realRoot = realpathSync(root)
+    const realFile = realpathSync(file)
+    const prefix = realRoot.endsWith(sep) ? realRoot : realRoot + sep
+    return realFile.startsWith(prefix)
+  } catch {
+    return false
+  }
 }
 
 export { sep } from 'path'

src/fs/fs.ts

@@ -9,8 +9,10 @@ export interface FS {
   readFileSync: (filepath: string) => string;
   /** resolve a file against directory, for given `ext` option */
   resolve: (dir: string, file: string, ext: string) => string;
-  /** check if file is contained in `root`, always return `true` by default. Warning: not setting this could expose path traversal vulnerabilities. */
-  contains?: (root: string, file: string) => boolean;
+  /** check if file is contained in `root`. Node default fs uses realpath; if omitted, loader assumes contained. */
+  contains?: (root: string, file: string) => Promise<boolean>;
+  /** sync check if file is contained in `root`, allows both renderSync and render. */
+  containsSync?: (root: string, file: string) => boolean;
   /** defaults to "/" */
   sep?: string;
   /** required for relative path resolving */

src/fs/loader.spec.ts

@@ -1,31 +1,34 @@
 import * as fs from './fs-impl'
-import { Loader } from './loader'
+import { resolve } from 'path'
+import { Loader, LookupType } from './loader'
+import { toValueSync } from '../util/async'
 
 describe('fs/loader', function () {
   describe('.candidates()', function () {
-    it('should resolve relatively', async function () {
+    it('should resolve relatively', function () {
       const loader = new Loader({ relativeReference: true, fs, extname: '' } as any)
-      const candidates = [...loader.candidates('./foo/bar', ['/root', '/root/foo'], '/root/current', true)]
-      expect(candidates).toContain('/root/foo/bar')
+      const candidates = [...loader.candidates('./foo/bar', ['/root', '/root/foo'], '/root/current')]
+      expect(candidates).toContain(resolve('/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).toHaveLength(0)
+  })
+  describe('.lookup()', function () {
+    it('should not include out of root candidates', function () {
+      const mockFs = { ...fs, existsSync: () => true, exists: async () => true }
+      const loader = new Loader({ relativeReference: true, fs: mockFs, extname: '', partials: ['/root'] } as any)
+      expect(() => toValueSync(loader.lookup('../foo/bar', LookupType.Partials, true, '/root/current')))
+        .toThrow(/ENOENT/)
     })
-    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).toHaveLength(0)
+    it('should treat root as a terminated path', function () {
+      const mockFs = { ...fs, existsSync: () => true, exists: async () => true }
+      const loader = new Loader({ relativeReference: true, fs: mockFs, extname: '', partials: ['/root'] } as any)
+      expect(() => toValueSync(loader.lookup('../root-dir/bar', LookupType.Partials, true, '/root/current')))
+        .toThrow(/ENOENT/)
     })
-    it('should default `.contains()` to () => true', async function () {
-      const customFs = {
-        ...fs,
-        contains: undefined
-      }
-      const loader = new Loader({ relativeReference: true, fs: customFs, extname: '' } as any)
-      const candidates = [...loader.candidates('../foo/bar', ['/root'], '/root/current', true)]
-      expect(candidates).toContain('/foo/bar')
+    it('should use permissive contains when fs.contains is omitted', function () {
+      const mockFs = { ...fs, existsSync: () => true, exists: async () => true, contains: undefined, containsSync: undefined }
+      const loader = new Loader({ relativeReference: true, fs: mockFs, extname: '', partials: ['/root'] } as any)
+      const result = toValueSync(loader.lookup('./foo/bar', LookupType.Partials, true, '/root/current'))
+      expect(result).toBe(resolve('/root/foo/bar'))
     })
   })
 })

src/fs/loader.ts

@@ -1,5 +1,5 @@
 import { FS } from './fs'
-import { assert } from '../util'
+import { assert, LiquidAsync, toLiquidAsync } from '../util'
 
 export interface LoaderOptions {
   fs: FS;
@@ -17,7 +17,8 @@ export enum LookupType {
 export class Loader {
   public shouldLoadRelative: (referencedFile: string) => boolean
   private options: LoaderOptions
-  private contains: (root: string, file: string) => boolean
+  private contains: LiquidAsync<NonNullable<FS['containsSync']>>
+  private exists: LiquidAsync<FS['existsSync']>
 
   constructor (options: LoaderOptions) {
     this.options = options
@@ -29,40 +30,48 @@ export class Loader {
     } else {
       this.shouldLoadRelative = (_referencedFile: string) => false
     }
-    this.contains = this.options.fs.contains || (() => true)
+    const fs = options.fs
+    this.contains = toLiquidAsync(
+      fs.contains?.bind(fs) || (async () => true),
+      fs.containsSync?.bind(fs) || (() => true)
+    )
+    this.exists = toLiquidAsync(
+      fs.exists?.bind(fs) || (async () => false),
+      fs.existsSync?.bind(fs)
+    )
   }
 
   public * lookup (file: string, type: LookupType, sync?: boolean, currentFile?: string): Generator<unknown, string, string> {
-    const { fs } = this.options
     const dirs = this.options[type]
-    for (const filepath of this.candidates(file, dirs, currentFile, type !== LookupType.Root)) {
-      if (sync ? fs.existsSync(filepath) : yield fs.exists(filepath)) return filepath
+    const enforceRoot = type !== LookupType.Root
+    for (const filepath of this.candidates(file, dirs, currentFile)) {
+      if (enforceRoot) {
+        let allowed = false
+        for (const dir of dirs) {
+          if (yield this.contains(!!sync, dir, filepath)) { allowed = true; break }
+        }
+        if (!allowed) continue
+      }
+      if (yield this.exists(!!sync, filepath)) return filepath
     }
     throw this.lookupError(file, dirs)
   }
 
-  public * candidates (file: string, dirs: string[], currentFile?: string, enforceRoot?: boolean) {
+  public * candidates (file: string, dirs: string[], currentFile?: string) {
     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)
-      if (isAllowed(referenced)) yield referenced
+      yield referenced
     }
     for (const dir of dirs) {
       const referenced = fs.resolve(dir, file, extname)
-      if (isAllowed(referenced)) yield referenced
+      yield referenced
     }
 
     if (fs.fallback !== undefined) {
       const filepath = fs.fallback(file)
-      if (filepath !== undefined && isAllowed(filepath)) yield filepath
+      if (filepath !== undefined) yield filepath
     }
   }
 

src/parser/parser.ts

@@ -1,4 +1,4 @@
-import { Limiter, toPromise, assert, isTagToken, isOutputToken, ParseError } from '../util'
+import { Limiter, toPromise, assert, isTagToken, isOutputToken, ParseError, toLiquidAsync, LiquidAsync } from '../util'
 import { Tokenizer } from './tokenizer'
 import { ParseStream } from './parse-stream'
 import { TopLevelToken, OutputToken } from '../tokens'
@@ -16,6 +16,7 @@ export class Parser {
   private cache?: LiquidCache
   private loader: Loader
   private parseLimit: Limiter
+  private readFile: LiquidAsync<FS['readFileSync']>
 
   public constructor (liquid: Liquid) {
     this.liquid = liquid
@@ -24,6 +25,10 @@ export class Parser {
     this.parseFile = this.cache ? this._parseFileCached : this._parseFile
     this.loader = new Loader(this.liquid.options)
     this.parseLimit = new Limiter('parse length', liquid.options.parseLimit)
+    this.readFile = toLiquidAsync(
+      this.fs.readFile?.bind(this.fs) || (async () => { throw new Error('readFile not implemented') }),
+      this.fs.readFileSync?.bind(this.fs)
+    )
   }
   public parse (html: string, filepath?: string): Template[] {
     html = String(html)
@@ -82,6 +87,6 @@ export class Parser {
   }
   private * _parseFile (file: string, sync?: boolean, type: LookupType = LookupType.Root, currentFile?: string): Generator<unknown, Template[], string> {
     const filepath = yield this.loader.lookup(file, type, sync, currentFile)
-    return this.parse(sync ? this.fs.readFileSync(filepath) : yield this.fs.readFile(filepath), filepath)
+    return this.parse(yield this.readFile(!!sync, filepath), filepath)
   }
 }

src/util/async.ts

@@ -1,5 +1,18 @@
 import { isPromise, isIterator } from './underscore'
 
+export type LiquidAsync<F extends (...args: any[]) => any> =
+  (sync: boolean, ...args: Parameters<F>) => ReturnType<F> | Promise<ReturnType<F>>
+
+export function toLiquidAsync<F extends (...args: any[]) => any> (
+  asyncFn: (...args: Parameters<F>) => Promise<ReturnType<F>>,
+  syncFn?: F
+): LiquidAsync<F> {
+  const syncImpl = syncFn || asyncFn as any
+  return (sync: boolean, ...args: any[]) => {
+    return sync ? syncImpl(...args as Parameters<F>) : asyncFn(...args as Parameters<F>)
+  }
+}
+
 // convert an async iterator to a Promise
 export async function toPromise<T> (val: Generator<unknown, T, unknown> | Promise<T> | T): Promise<T> {
   if (!isIterator(val)) return val