diff --git a/.cursor/rules/architecture.mdc b/.cursor/rules/architecture.mdc new file mode 100644 index 000000000..d83fabffa --- /dev/null +++ b/.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` — 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`. diff --git a/.cursor/rules/conventions.mdc b/.cursor/rules/conventions.mdc new file mode 100644 index 000000000..55febef8e --- /dev/null +++ b/.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. diff --git a/docs/source/tutorials/render-file.md b/docs/source/tutorials/render-file.md index 0f636852f..6701882c6 100644 --- a/docs/source/tutorials/render-file.md +++ b/docs/source/tutorials/render-file.md @@ -92,7 +92,7 @@ var engine = new Liquid({ }); ``` -{% note warn Path Traversal Vulnerability %}The default value of contains() always returns true. That means when specifying an abstract file system, you'll need to provide a proper contains() to avoid expose such vulnerabilities.{% endnote %} +{% note warn Path Traversal Vulnerability %}The built-in Node fs implements contains() with realpath so templates cannot escape the root via symlinks. The browser bundle omits contains (loader treats paths as allowed). For a custom abstract fs, implement contains unless every resolved path is trusted.{% endnote %} ## In-memory Template diff --git a/src/fs/fs-impl.spec.ts b/src/fs/fs-impl.spec.ts index 4881cec74..1258ca78e 100644 --- a/src/fs/fs-impl.spec.ts +++ b/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 }) + } + }) + }) }) diff --git a/src/fs/fs-impl.ts b/src/fs/fs-impl.ts index d542c1916..ea167a8d6 100644 --- a/src/fs/fs-impl.ts +++ b/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' diff --git a/src/fs/fs.ts b/src/fs/fs.ts index 0a0104591..15f16eb8b 100644 --- a/src/fs/fs.ts +++ b/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; + /** 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 */ diff --git a/src/fs/loader.spec.ts b/src/fs/loader.spec.ts index b694ef1d3..81116b2d9 100644 --- a/src/fs/loader.spec.ts +++ b/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')) }) }) }) diff --git a/src/fs/loader.ts b/src/fs/loader.ts index 2c58e231c..b0e471e11 100644 --- a/src/fs/loader.ts +++ b/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> + private exists: LiquidAsync 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 { - 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 } } diff --git a/src/parser/parser.ts b/src/parser/parser.ts index 2c02dc407..f61f2323e 100644 --- a/src/parser/parser.ts +++ b/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 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 { 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) } } diff --git a/src/util/async.ts b/src/util/async.ts index 9d836cd42..c190cd860 100644 --- a/src/util/async.ts +++ b/src/util/async.ts @@ -1,5 +1,18 @@ import { isPromise, isIterator } from './underscore' +export type LiquidAsync any> = + (sync: boolean, ...args: Parameters) => ReturnType | Promise> + +export function toLiquidAsync any> ( + asyncFn: (...args: Parameters) => Promise>, + syncFn?: F +): LiquidAsync { + const syncImpl = syncFn || asyncFn as any + return (sync: boolean, ...args: any[]) => { + return sync ? syncImpl(...args as Parameters) : asyncFn(...args as Parameters) + } +} + // convert an async iterator to a Promise export async function toPromise (val: Generator | Promise | T): Promise { if (!isIterator(val)) return val diff --git a/test/e2e/parse-and-render.spec.ts b/test/e2e/parse-and-render.spec.ts index 43bf3c162..cfca9c2b2 100644 --- a/test/e2e/parse-and-render.spec.ts +++ b/test/e2e/parse-and-render.spec.ts @@ -1,4 +1,7 @@ import { Liquid } from '../..' +import { mkdtempSync, writeFileSync, symlinkSync, rmSync } from 'fs' +import { join } from 'path' +import { tmpdir } from 'os' describe('.parseAndRender()', function () { var engine: Liquid, strictEngine: Liquid @@ -57,4 +60,26 @@ describe('.parseAndRender()', function () { const html = await engine.parseAndRender(src) expect(html).toBe('true') }) + const canSymlink = process.platform !== 'win32' + ;(canSymlink ? describe : describe.skip)('symlink outside root', function () { + let root: string, secret: string + beforeAll(function () { + root = mkdtempSync(join(tmpdir(), 'liquid-e2e-root-')) + secret = join(tmpdir(), `liquid-e2e-secret-${Date.now()}.liquid`) + writeFileSync(secret, 'SECRET_OUTSIDE') + symlinkSync(secret, join(root, 'link.liquid')) + }) + afterAll(function () { + rmSync(root, { recursive: true, force: true }) + rmSync(secret, { force: true }) + }) + it('should not render a symlink partial whose target is outside root', async function () { + const e = new Liquid({ root: [root], extname: '.liquid', relativeReference: false }) + await expect(e.parseAndRender('{% render "link" %}')).rejects.toThrow(/ENOENT|Failed to lookup/) + }) + it('should not render a symlink partial via parseAndRenderSync', function () { + const e = new Liquid({ root: [root], extname: '.liquid', relativeReference: false }) + expect(() => e.parseAndRenderSync('{% render "link" %}')).toThrow(/ENOENT|Failed to lookup/) + }) + }) }) diff --git a/test/stub/mockfs.ts b/test/stub/mockfs.ts index bd3c77717..4c9d4cea3 100644 --- a/test/stub/mockfs.ts +++ b/test/stub/mockfs.ts @@ -1,6 +1,6 @@ import { isString, forOwn } from '../../src/util/underscore' import * as fs from '../../src/fs/fs-impl' -import { resolve } from 'path' +import { resolve, sep } from 'path' interface FileDescriptor { mode: string; @@ -8,7 +8,7 @@ interface FileDescriptor { } let files: { [path: string]: FileDescriptor } = {} -const { readFile, exists, readFileSync, existsSync } = fs +const { readFile, exists, readFileSync, existsSync, contains, containsSync } = fs export function mock (options: { [path: string]: (string | FileDescriptor) }) { forOwn(options, (val, key) => { @@ -30,6 +30,16 @@ export function mock (options: { [path: string]: (string | FileDescriptor) }) { }; (fs as any).existsSync = function (path: string) { return !!files[path] + }; + (fs as any).contains = async (root: string, file: string) => { + root = resolve(root) + if (!root.endsWith(sep)) root += sep + return file.startsWith(root) + }; + (fs as any).containsSync = (root: string, file: string) => { + root = resolve(root) + if (!root.endsWith(sep)) root += sep + return file.startsWith(root) } } @@ -38,5 +48,7 @@ export function restore () { (fs as any).readFileSync = readFileSync; (fs as any).existsSync = existsSync; (fs as any).readFile = readFile; - (fs as any).exists = exists + (fs as any).exists = exists; + (fs as any).contains = contains; + (fs as any).containsSync = containsSync }