From 9eab70be95474ce58bd049f0db866a3924951088 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 1 Sep 2026 12:41:26 +0200 Subject: [PATCH] refactor: use github.com/go-webengine/esbuildsandbox instead of a local copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ResolveDir sandboxing fix (engine#65) is a genuinely reusable pattern: any Go program driving evanw/esbuild's pkg/api through a plugin has the same exposure — a glob-shaped dynamic import bypasses OnResolve/OnLoad entirely and resolves against ResolveDir on the real filesystem, no matter how thorough the plugin's own sandboxing is. Extracted it into its own package (github.com/go-webengine/esbuildsandbox, v0.1.0) rather than leaving it siloed here, with its own README explaining the vulnerability and a self-contained regression test that drives a real api.Build and proves it (confirmed to time out walking real "/" when reverted). modules.go now calls esbuildsandbox.ResolveDir() at all three call sites instead of the local esbuildSandboxResolveDir()/esbuildSandboxOnce/ esbuildSandboxDir, which are deleted. Removed the now-redundant TestEsbuildSandboxResolveDirIsIsolated (the shared package tests that itself, with 100% coverage) but kept TestModuleGlobImportDoesNotWalk- RealFilesystem — it is integration-level on purpose, proving this engine's OWN plugin wiring actually uses the shared sandbox at every call site, not just that the sandbox itself is safe in isolation. No behavioural change: same directory-creation logic, same memoization, same fallback. Verified live against developer.mozilla.org (~550ms, matching the fix's original numbers) and the full test suite. --- go.mod | 1 + go.sum | 2 ++ modules.go | 46 ++++------------------------------------------ modules_test.go | 49 +++++++++++++------------------------------------ 4 files changed, 20 insertions(+), 78 deletions(-) diff --git a/go.mod b/go.mod index 2e452fe..e348211 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/go-images/images v0.0.0-20260814204114-5f83e6345a72 github.com/go-opentype/fonts v0.5.0 github.com/go-opentype/opentype v0.5.0 + github.com/go-webengine/esbuildsandbox v0.1.0 github.com/go-widgets/painter v0.12.0 github.com/srwiley/oksvg v0.0.0-20221011165216-be6e8873101c github.com/srwiley/rasterx v0.0.0-20220730225603-2ab79fcdd4ef diff --git a/go.sum b/go.sum index 3c194a8..cae4270 100644 --- a/go.sum +++ b/go.sum @@ -26,6 +26,8 @@ github.com/go-opentype/opentype v0.5.0 h1:++VoqgbXgYACyTTNzUAivoxW9M3m2gxWMAn6bI github.com/go-opentype/opentype v0.5.0/go.mod h1:AOixevJf7XQaH7+WG+OMIOZEbYPXfMqklVk26Y6YTUU= github.com/go-sourcemap/sourcemap v2.1.4+incompatible h1:a+iTbH5auLKxaNwQFg0B+TCYl6lbukKPc7b5x0n1s6Q= github.com/go-sourcemap/sourcemap v2.1.4+incompatible/go.mod h1:F8jJfvm2KbVjc5NqelyYJmf/v5J0dwNLS2mL4sNA1Jg= +github.com/go-webengine/esbuildsandbox v0.1.0 h1:1LdmX+0/yJarQlxZgJ7JIMu+Qy6P7Yzu4NFcX9+jFl8= +github.com/go-webengine/esbuildsandbox v0.1.0/go.mod h1:za8oj1vbDfVkTpHLeRk7JfEQjmJfj+GHNq4R47681vQ= github.com/go-widgets/painter v0.12.0 h1:gMqTVbIbGSKmaE28NKsbbKdNIk2drupNMNuLjFP6T6I= github.com/go-widgets/painter v0.12.0/go.mod h1:IPRLqdUJuJX8sfuHeYLZCzjoLvA0ApbOlyIAVmguJDQ= github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= diff --git a/modules.go b/modules.go index 4b2cda4..8110ec7 100644 --- a/modules.go +++ b/modules.go @@ -8,12 +8,12 @@ import ( "fmt" "io" "net/http" - "os" "strings" "sync" "time" "github.com/evanw/esbuild/pkg/api" + "github.com/go-webengine/esbuildsandbox" "github.com/go-webengine/engine/dom" ) @@ -203,14 +203,14 @@ func (e *Engine) bundleModuleScripts(ctx context.Context, doc *Document) (string mu.Lock() failed++ mu.Unlock() - return api.OnLoadResult{Contents: &empty, Loader: api.LoaderJS, ResolveDir: esbuildSandboxResolveDir()}, nil + return api.OnLoadResult{Contents: &empty, Loader: api.LoaderJS, ResolveDir: esbuildsandbox.ResolveDir()}, nil } mu.Lock() fetched++ bytesIn += len(src) mu.Unlock() contents := src - return api.OnLoadResult{Contents: &contents, Loader: api.LoaderJS, ResolveDir: esbuildSandboxResolveDir()}, nil + return api.OnLoadResult{Contents: &contents, Loader: api.LoaderJS, ResolveDir: esbuildsandbox.ResolveDir()}, nil }) }, } @@ -228,7 +228,7 @@ func (e *Engine) bundleModuleScripts(ctx context.Context, doc *Document) (string return api.Build(api.BuildOptions{ Stdin: &api.StdinOptions{ Contents: entrySrc.String(), - ResolveDir: esbuildSandboxResolveDir(), + ResolveDir: esbuildsandbox.ResolveDir(), Sourcefile: doc.URL, Loader: api.LoaderJS, }, @@ -270,44 +270,6 @@ func (e *Engine) bundleModuleScripts(ctx context.Context, doc *Document) (string return out, stats, true } -// esbuildSandboxDir and esbuildSandboxOnce back esbuildSandboxResolveDir. -var ( - esbuildSandboxOnce sync.Once - esbuildSandboxDir string -) - -// esbuildSandboxResolveDir returns a directory that stays empty for the life -// of the process, used as esbuild's Stdin.ResolveDir instead of the real -// filesystem root. -// -// The webengine-http plugin above intercepts ordinary import resolution, but -// esbuild's bundler special-cases a glob import — `import(`./locales/${lang} -// .js`)`, ordinary output from many bundlers' locale/route code-splitting, no -// wrongdoing needed on the page's part — by resolving it directly against -// ResolveDir on the REAL filesystem, entirely bypassing OnResolve. A page's -// entry script runs with Stdin.ResolveDir as its own directory, so serving -// that as "/" (as this used to) let a page merely containing such an import -// make the engine recursively walk the host's real root filesystem: observed -// live on a production MDN page, ~9s spent almost entirely in readdir/ -// symlink syscalls (github.com/evanw/esbuild/internal/fs, via pprof) for a -// single render, and — independent of the cost — the host's real directory -// tree is not something a remote page should ever cause this engine to read. -// An isolated, always-empty directory makes every such glob resolve to zero -// matches immediately instead, like a browser's own sandboxed module loader. -func esbuildSandboxResolveDir() string { - esbuildSandboxOnce.Do(func() { - dir, err := os.MkdirTemp("", "webengine-esbuild-sandbox-") - if err != nil { - // The OS temp directory is not guaranteed empty, but it is still a - // vastly smaller, more contained tree than the real root — better - // degraded behaviour than reintroducing the original bug. - dir = os.TempDir() - } - esbuildSandboxDir = dir - }) - return esbuildSandboxDir -} - // resolveModuleSpecifier resolves an import specifier against its importer (or // the page URL when importing from the virtual entry), producing an absolute // http(s) URL. Bare specifiers (no leading /, ./, ../ or scheme) are rejected — diff --git a/modules_test.go b/modules_test.go index 9b1aa90..04da2c5 100644 --- a/modules_test.go +++ b/modules_test.go @@ -8,7 +8,6 @@ import ( "image" "net/http" "net/http/httptest" - "os" "strings" "testing" "time" @@ -357,33 +356,6 @@ func TestModuleBundleAbandonedOnBudget(t *testing.T) { } } -// TestEsbuildSandboxResolveDirIsIsolated proves the directory backing -// esbuild's ResolveDir is a real, existing, empty directory distinct from the -// filesystem root — the mechanism esbuildSandboxResolveDir relies on to make -// a glob import resolve to nothing instead of walking real content. -func TestEsbuildSandboxResolveDirIsIsolated(t *testing.T) { - dir := esbuildSandboxResolveDir() - if dir == "" || dir == "/" { - t.Fatalf("sandbox dir = %q, want a real non-root path", dir) - } - info, err := os.Stat(dir) - if err != nil || !info.IsDir() { - t.Fatalf("sandbox dir does not exist as a directory: %v", err) - } - entries, err := os.ReadDir(dir) - if err != nil { - t.Fatalf("read sandbox dir: %v", err) - } - if len(entries) != 0 { - t.Errorf("sandbox dir has %d entries, want empty", len(entries)) - } - // Memoized for the process lifetime: a second call must return the same - // path, not mint a fresh temp directory every bundle. - if got := esbuildSandboxResolveDir(); got != dir { - t.Errorf("second call returned %q, want the memoized %q", got, dir) - } -} - // moduleGlobFixtureServer serves an entry module containing a dynamic import // with a template-literal glob pattern whose variable segment is the FIRST // path component (`./${id}/index.js`) — the shape of a common real-world @@ -409,14 +381,19 @@ func moduleGlobFixtureServer() *httptest.Server { } // TestModuleGlobImportDoesNotWalkRealFilesystem is the regression test for the -// bug fixed by esbuildSandboxResolveDir: before it, every OnLoadResult (and -// the Stdin entry) declared ResolveDir "/", so a page merely containing a -// glob-shaped dynamic import made the engine recursively walk the host's real -// root filesystem — observed live rendering a production MDN page, ~9s spent -// almost entirely in readdir/symlink syscalls for a single render (pprof). -// With an isolated, empty ResolveDir the glob resolves to zero matches -// immediately, so this must complete well within a tight bound rather than -// stall the bundle budget. +// bug fixed by using github.com/go-webengine/esbuildsandbox's ResolveDir: +// before it, every OnLoadResult (and the Stdin entry) in this file declared +// ResolveDir "/", so a page merely containing a glob-shaped dynamic import +// made the engine recursively walk the host's real root filesystem — +// observed live rendering a production MDN page, ~9s spent almost entirely +// in readdir/symlink syscalls for a single render (pprof). This test is +// integration-level on purpose: esbuildsandbox's own package tests prove its +// ResolveDir itself is safe, but only this test proves bundleModuleScripts +// actually WIRES it into every ResolveDir esbuild sees here, so a future edit +// reintroducing a stray "/" at one of the three call sites in modules.go +// would still be caught. With the sandbox in place the glob resolves to zero +// matches immediately, so this must complete well within a tight bound +// rather than stall the bundle budget. func TestModuleGlobImportDoesNotWalkRealFilesystem(t *testing.T) { srv := moduleGlobFixtureServer() defer srv.Close()