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

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
     }
   }