fix(js): respect allow-local-file-access in require#7332
Merged
dwisiswant0 merged 2 commits intodevfrom Apr 10, 2026
Merged
Conversation
The goja `require() `function used the default host filesystem loader which let JavaScript templates import any local files even when `allow-local-file-access` was disabled. Pooled runtimes kept `require()` state around so a module loaded during a privileged execution could remain cached for a later restricted one. Rebuild the require registry per execution after setting the execution context, and route file- backed module loads to preserve native modules while enforcing the same sandbox rules (as `nuclei/fs`). Signed-off-by: Dwi Siswanto <git@dw1.io>
Neo - PR Security ReviewNo security issues found Comment |
Contributor
WalkthroughRefactors per-runtime JavaScript Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Execution Runner
participant Goja as goja.Runtime
participant SourceLoader as require.SourceLoader
participant Protocol as protocolstate
participant FS as Filesystem
Runner->>Goja: create new runtime, set context (executionId, ctx)
Runner->>Goja: enableRequire(runtime)
enableRequire->>SourceLoader: newSourceLoader(runtime) (captures executionId)
Goja->>SourceLoader: require("modulePath")
SourceLoader->>Protocol: NormalizePathWithExecutionId(modulePath, executionId)
Protocol->>FS: resolve path, check IsPathWithinDirectory(templateDir)
FS-->>Protocol: resolved normalized path
Protocol-->>SourceLoader: normalized filesystem path or error
SourceLoader->>FS: read module file (if permitted)
FS-->>SourceLoader: module source or permission error
SourceLoader-->>Goja: provide module source or throw
Goja->>Runner: execution result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai create an issue for this and link it to this PR, and it must be bug-oriented, also starts with "[BUG] " for the issue title. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 54
Issue summary:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/compiler/pool.go`:
- Around line 239-253: The newSourceLoader's current behavior silently
substitutes an empty executionId when runtime.GetContextValue("executionId") is
missing, causing NormalizePathWithExecutionId to treat the call as a normal
LFA-disabled path; change this to fail fast by returning an error when the
executionId is absent or not a string. In newSourceLoader (require.SourceLoader
closure) check runtime.GetContextValue("executionId") and if ok==false or the
value isn't a string, return a descriptive error (rather than continuing with
executionID=""), then call protocolstate.NormalizePathWithExecutionId and
require.DefaultSourceLoader as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b2391c9-7216-4f35-87d6-e14680443aae
📒 Files selected for processing (2)
pkg/js/compiler/compiler_test.gopkg/js/compiler/pool.go
Replace lexical prefix checks in the template file sandbox with a shared path containment helper that canonicalizes both paths before comparing them to prevent false rejections when the configured templates directory and the resolved file path differ only due to symlink expansion on macOS or path normalization on Windows. Apply the helper in `protocolstate.NormalizePath()` and `Options.GetValidAbsPath()` so JS `require()`- based module loads and helper file resolution use the same rules. Signed-off-by: Dwi Siswanto <git@dw1.io>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/types/types.go (1)
900-901:⚠️ Potential issue | 🔴 CriticalReplace remaining
HasPrefixpath checks; they are sandbox-bypass prone.Line 900 and Line 925 still use lexical prefix checks for path boundaries. That can incorrectly allow sibling paths (e.g.,
/home/u/templates2vs/home/u/templates) and weakens local-file restrictions. Replace withfilepathutil.IsPathWithinDirectory(), which correctly canonicalizes paths (handling symlinks and absolute paths) and uses semantic path relationships viafilepath.Rel()rather than unsafe string prefixing.🔧 Proposed fix
@@ - if isHomeDir(helperFilePath) && isHomeDir(templatePath) && strings.HasPrefix(filepath.Dir(helperFilePath), filepath.Dir(templatePath)) { - return helperFilePath, nil - } + if isHomeDir(helperFilePath) && isHomeDir(templatePath) { + helperDir := filepath.Dir(helperFilePath) + templateDir := filepath.Dir(templatePath) + if filepathutil.IsPathWithinDirectory(helperDir, templateDir) { + return helperFilePath, nil + } + } @@ func isHomeDir(path string) bool { homeDir := folderutil.HomeDirOrDefault("") - return strings.HasPrefix(path, homeDir) + return filepathutil.IsPathWithinDirectory(path, homeDir) }Also applies to: 923-925
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/types.go` around lines 900 - 901, The current lexical path check using strings.HasPrefix(filepath.Dir(helperFilePath), filepath.Dir(templatePath)) is unsafe; update the conditional(s) that compare helperFilePath and templatePath (including the other occurrence near the same block) to use filepathutil.IsPathWithinDirectory (or equivalent canonicalizing helper) to determine semantic containment after resolving paths/symlinks—replace the HasPrefix-based checks with calls to filepathutil.IsPathWithinDirectory(templatePath, helperFilePath) (or the library's argument order) so the code in the helper/template validation branches uses canonicalized path containment rather than string prefixing.
🧹 Nitpick comments (1)
pkg/utils/filepath/filepath_test.go (1)
33-55: Add a negative symlink-escape test case for regression safety.Given this helper is security-sensitive, please also assert that a symlink inside the base directory pointing outside is rejected.
🧪 Proposed additional test
+func TestIsPathWithinDirectoryRejectsSymlinkEscape(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliable on all Windows runners") + } + + baseDir := t.TempDir() + outsideDir := t.TempDir() + outsideFile := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(outsideFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + + linkPath := filepath.Join(baseDir, "escape") + if err := os.Symlink(outsideDir, linkPath); err != nil { + t.Fatalf("create symlink: %v", err) + } + + escapedPath := filepath.Join(linkPath, "secret.txt") + if IsPathWithinDirectory(escapedPath, baseDir) { + t.Fatalf("expected %q to be outside %q via symlink escape", escapedPath, baseDir) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/filepath/filepath_test.go` around lines 33 - 55, Add a negative symlink-escape test to ensure IsPathWithinDirectory rejects symlinks inside the base that point outside: in the TestIsPathWithinDirectoryWithSymlinkedDirectory test (or a new test nearby), create an external temp file or dir outside realDir, then inside the aliasDir/base create a symlink (e.g., aliasDir/escape -> outsidePath) and assert IsPathWithinDirectory(escapePath, aliasDir) returns false (and fail the test if it returns true); this uses the existing helper IsPathWithinDirectory and mirrors the positive symlink test structure but verifies rejection of symlink escape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/types/types.go`:
- Around line 900-901: The current lexical path check using
strings.HasPrefix(filepath.Dir(helperFilePath), filepath.Dir(templatePath)) is
unsafe; update the conditional(s) that compare helperFilePath and templatePath
(including the other occurrence near the same block) to use
filepathutil.IsPathWithinDirectory (or equivalent canonicalizing helper) to
determine semantic containment after resolving paths/symlinks—replace the
HasPrefix-based checks with calls to
filepathutil.IsPathWithinDirectory(templatePath, helperFilePath) (or the
library's argument order) so the code in the helper/template validation branches
uses canonicalized path containment rather than string prefixing.
---
Nitpick comments:
In `@pkg/utils/filepath/filepath_test.go`:
- Around line 33-55: Add a negative symlink-escape test to ensure
IsPathWithinDirectory rejects symlinks inside the base that point outside: in
the TestIsPathWithinDirectoryWithSymlinkedDirectory test (or a new test nearby),
create an external temp file or dir outside realDir, then inside the
aliasDir/base create a symlink (e.g., aliasDir/escape -> outsidePath) and assert
IsPathWithinDirectory(escapePath, aliasDir) returns false (and fail the test if
it returns true); this uses the existing helper IsPathWithinDirectory and
mirrors the positive symlink test structure but verifies rejection of symlink
escape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a68514d-de8d-4e2f-beb9-2400d85ef715
📒 Files selected for processing (6)
pkg/js/compiler/compiler_test.gopkg/protocols/common/protocolstate/file.gopkg/types/types.gopkg/utils/filepath/doc.gopkg/utils/filepath/filepath.gopkg/utils/filepath/filepath_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/utils/filepath/doc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/js/compiler/compiler_test.go
Mzack9999
approved these changes
Apr 9, 2026
dwisiswant0
deleted the
dwisiswant0/fix/js/respect-allow-local-file-access-in-require
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
fix(js): respect
allow-local-file-accessinrequireThe goja
require()function used the defaulthost filesystem loader which let JavaScript
templates import any local files even when
allow-local-file-accesswas disabled.Pooled runtimes kept
require()state around soa module loaded during a privileged execution
could remain cached for a later restricted one.
Rebuild the require registry per execution after
setting the execution context, and route file-
backed module loads to preserve native modules
while enforcing the same sandbox rules (as
nuclei/fs).Proof
Template:
this patch:
dev:
Checklist
Summary by CodeRabbit
Tests
Refactor
Bug Fix / Security
Documentation