refactor: use path-aware filesystem containment checks - #7420
Merged
ehsandeep merged 5 commits intoJun 4, 2026
Merged
Conversation
Signed-off-by: Dwi Siswanto <git@dw1.io>
Member
Author
|
@coderabbitai create an issue for this. prefixed with "[BUG]". it's about correctness. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 55
[BUG] Incorrect lexical prefix checks allow path containment bypass in filesystem trust boundaries — it covers the correctness problem across all five affected areas (helper payload loading, template archive extraction, custom template metadata, template path classification, and headless screenshot output validation), with a backlink to this PR. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplace lexical prefix-based path checks with canonical filesystem containment checks (IsPathWithinDirectory / IsPathWithinAnyDirectory) across utilities, catalog, installer, external custom template downloaders, headless screenshot gating, helper validation, and reporting; add tests and fuzz coverage. ChangesPath-Containment Security Fixes
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related Issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/protocols/headless/engine/page_actions.go (1)
526-559:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate the final screenshot path, not the pre-extension input.
Line 531 checks
to, but Lines 551-553 can still move the write outsidecwdby appending.pngafterward. For example,to="."passes the containment check and then becomes<cwd>.png, which is a sibling path outside the allowed directory.🔒 Proposed fix
to, err = fileutil.CleanPath(to) if err != nil { return errkit.Newf("could not clean output screenshot path %s", to) } - if err := p.isScreenshotPathAllowed(to); err != nil { - return err - } + filePath := to + if !strings.HasSuffix(filePath, ".png") { + filePath += ".png" + } + if err := p.isScreenshotPathAllowed(filePath); err != nil { + return err + } mkdir, err := p.getActionArg(act, "mkdir") if err != nil { return err } @@ - if mkdir == "true" && stringsutil.ContainsAny(to, folderutil.UnixPathSeparator, folderutil.WindowsPathSeparator) { + if mkdir == "true" && stringsutil.ContainsAny(filePath, folderutil.UnixPathSeparator, folderutil.WindowsPathSeparator) { // creates new directory if needed based on path `to` // TODO: replace all permission bits with fileutil constants (https://github.com/projectdiscovery/utils/issues/113) - if err := os.MkdirAll(filepath.Dir(to), 0700); err != nil { + if err := os.MkdirAll(filepath.Dir(filePath), 0700); err != nil { return errkit.Wrap(err, "failed to create directory while writing screenshot") } } - - // actual file path to write - filePath := to - if !strings.HasSuffix(filePath, ".png") { - filePath += ".png" - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/protocols/headless/engine/page_actions.go` around lines 526 - 559, The code currently validates the input `to` before appending the ".png" extension, which lets inputs like "." slip past containment checks; update the logic to build the final path first (variable `filePath`), appending ".png" if needed, then call `p.isScreenshotPathAllowed(filePath)` to validate the actual write target; also use `filepath.Dir(filePath)` when creating directories (the `mkdir` branch) so directory creation and the containment check operate on the same final path before calling `os.WriteFile`.pkg/types/types.go (1)
886-900:⚠️ Potential issue | 🟠 MajorFix helper-path resolution to avoid process CWD dependence
Inpkg/types/types.go’sGetValidAbsPath, the fallback branch doesfileutil.CleanPath(helperFilePath);fileutil.CleanPathresolves non-absolute paths usingos.Getwd()(process CWD). Relative helper references passed from templates are not normalized againstfilepath.Dir(templatePath)beforeLoadHelperFile(payloadspass the raw string;ImportFileRefsonly gates viafileutil.FileExists(request.Source/request.Code)on the raw value). This means a helper likepayloads.txtintended to be relative to the template directory will be interpreted relative to CWD wheneverResolveNClean(...GetTemplateDir())doesn’t succeed.
- Resolve relative helper paths against
filepath.Dir(templatePath)(or otherwise pass an explicit base) before sandbox checks, instead of relying onfileutil.CleanPath’s CWD behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/types.go` around lines 886 - 900, GetValidAbsPath currently calls fileutil.CleanPath(helperFilePath) which resolves relative paths against process CWD; instead, detect non-absolute helperFilePath in GetValidAbsPath and join it with filepath.Dir(templatePath) (or call ResolveNClean with the template dir as base) before cleaning and sandbox/existence checks so helper references like "payloads.txt" are resolved relative to the template directory; update any subsequent uses (e.g., LoadHelperFile, ImportFileRefs) to use this resolved path and preserve existing error wrapping via errkit.Wrapf.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/installer/template.go`:
- Around line 281-283: The path containment check currently calls
filepathutil.IsPathWithinDirectory on newPath and filepath.Dir(newPath), but
both can be non-existent and miss a symlinked ancestor; change the check to walk
upward from newPath to the nearest existing ancestor (using os.Stat or similar)
and call filepathutil.IsPathWithinDirectory against that existing parent and
templateDir before proceeding; update the code paths that call
CreateFolder/WriteFile to rely on this new validation and add a regression test
that creates a symlinked child directory (e.g., templateDir/link -> /outside)
and asserts that operations like CreateFolder/WriteFile are rejected.
---
Outside diff comments:
In `@pkg/protocols/headless/engine/page_actions.go`:
- Around line 526-559: The code currently validates the input `to` before
appending the ".png" extension, which lets inputs like "." slip past containment
checks; update the logic to build the final path first (variable `filePath`),
appending ".png" if needed, then call `p.isScreenshotPathAllowed(filePath)` to
validate the actual write target; also use `filepath.Dir(filePath)` when
creating directories (the `mkdir` branch) so directory creation and the
containment check operate on the same final path before calling `os.WriteFile`.
In `@pkg/types/types.go`:
- Around line 886-900: GetValidAbsPath currently calls
fileutil.CleanPath(helperFilePath) which resolves relative paths against process
CWD; instead, detect non-absolute helperFilePath in GetValidAbsPath and join it
with filepath.Dir(templatePath) (or call ResolveNClean with the template dir as
base) before cleaning and sandbox/existence checks so helper references like
"payloads.txt" are resolved relative to the template directory; update any
subsequent uses (e.g., LoadHelperFile, ImportFileRefs) to use this resolved path
and preserve existing error wrapping via errkit.Wrapf.
🪄 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: e38b910f-424b-4dd6-8b22-008476bc4461
📒 Files selected for processing (15)
pkg/catalog/config/nucleiconfig.gopkg/catalog/config/nucleiconfig_test.gopkg/catalog/config/template.gopkg/catalog/config/template_test.gopkg/installer/template.gopkg/installer/template_test.gopkg/installer/zipslip_unix_test.gopkg/protocols/headless/engine/page_actions.gopkg/protocols/headless/engine/page_actions_test.gopkg/types/types.gopkg/types/types_test.gopkg/utils/filepath/filepath.gopkg/utils/filepath/filepath_test.gopkg/utils/template_path.gopkg/utils/template_path_test.go
Comment on lines
+281
to
283
| if !filepathutil.IsPathWithinDirectory(newPath, templateDir) || !filepathutil.IsPathWithinDirectory(filepath.Dir(newPath), templateDir) { | ||
| // we don't allow LFI | ||
| return "" |
Contributor
There was a problem hiding this comment.
Validate against the nearest existing ancestor, not just filepath.Dir(newPath).
For entries like root/link/new/file.yaml, both newPath and filepath.Dir(newPath) can be non-existent. If templateDir/link already exists as a symlink outside the templates tree, the later CreateFolder/WriteFile path will still follow it and escape the containment boundary. Please walk up to the first existing parent before calling IsPathWithinDirectory, and add a regression test for a symlinked child directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/installer/template.go` around lines 281 - 283, The path containment check
currently calls filepathutil.IsPathWithinDirectory on newPath and
filepath.Dir(newPath), but both can be non-existent and miss a symlinked
ancestor; change the check to walk upward from newPath to the nearest existing
ancestor (using os.Stat or similar) and call filepathutil.IsPathWithinDirectory
against that existing parent and templateDir before proceeding; update the code
paths that call CreateFolder/WriteFile to rely on this new validation and add a
regression test that creates a symlinked child directory (e.g., templateDir/link
-> /outside) and asserts that operations like CreateFolder/WriteFile are
rejected.
Several filesystem trust boundaries relied on lexical prefix checks to decide whether a path fell under an allowed directory. That let sibling paths be treated as if they were children of trusted directories. Replace those checks with canonical path containment checks for helper payload loading, template archive extraction, custom template metadata, template path classification, and headless screenshot output validation. Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
force-pushed
the
dwisiswant0/refactor/use-path-aware-filesystem-containment-checks
branch
from
May 22, 2026 22:29
fd9bb10 to
85574ef
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/utils/filepath/filepath_test.go (1)
71-75: ⚡ Quick winConsider adding a test for sibling directory prefix bypass.
The primary vulnerability this PR addresses—sibling paths like
/trusted-dir-evilincorrectly matching/trusted-dirwith lexical prefix checks—isn't directly tested in this utility's test file. While downstream tests may cover this, adding a unit test here would document the security invariant at the source.Suggested test case
+func TestIsPathWithinDirectory_SiblingPrefix(t *testing.T) { + baseDir := t.TempDir() // e.g., /tmp/TestXXX123 + siblingDir := baseDir + "-evil" + if err := os.MkdirAll(siblingDir, 0o755); err != nil { + t.Fatal(err) + } + siblingFile := filepath.Join(siblingDir, "payload.txt") + if err := os.WriteFile(siblingFile, []byte("malicious"), 0o600); err != nil { + t.Fatal(err) + } + + if IsPathWithinDirectory(siblingFile, baseDir) { + t.Fatalf("sibling path %q should NOT be inside %q", siblingFile, baseDir) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/utils/filepath/filepath_test.go` around lines 71 - 75, Add a unit test in filepath_test.go that asserts IsPathWithinDirectory correctly rejects sibling-directory prefix bypasses: construct a realDir (e.g., "/trusted-dir") and an aliasDir or test path that is a lexical sibling prefix (e.g., "/trusted-dir-evil" or filepath.Join(realDir+"-evil", "file.js")) and verify IsPathWithinDirectory returns false; reference the existing test variables/flow around IsPathWithinDirectory, aliasDir, realDir and missingChildFile to place the new case nearby so the security invariant is documented at the source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/utils/filepath/filepath_test.go`:
- Around line 71-75: Add a unit test in filepath_test.go that asserts
IsPathWithinDirectory correctly rejects sibling-directory prefix bypasses:
construct a realDir (e.g., "/trusted-dir") and an aliasDir or test path that is
a lexical sibling prefix (e.g., "/trusted-dir-evil" or
filepath.Join(realDir+"-evil", "file.js")) and verify IsPathWithinDirectory
returns false; reference the existing test variables/flow around
IsPathWithinDirectory, aliasDir, realDir and missingChildFile to place the new
case nearby so the security invariant is documented at the source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b926836-ecdf-428b-8ace-812853318b7c
📒 Files selected for processing (15)
pkg/catalog/config/nucleiconfig.gopkg/catalog/config/nucleiconfig_test.gopkg/catalog/config/template.gopkg/catalog/config/template_test.gopkg/installer/template.gopkg/installer/template_test.gopkg/installer/zipslip_unix_test.gopkg/protocols/headless/engine/page_actions.gopkg/protocols/headless/engine/page_actions_test.gopkg/types/types.gopkg/types/types_test.gopkg/utils/filepath/filepath.gopkg/utils/filepath/filepath_test.gopkg/utils/template_path.gopkg/utils/template_path_test.go
Mzack9999
approved these changes
May 25, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/external/customtemplates/azure_blob.go (1)
92-101: ⚡ Quick winAdd Azure regression coverage for unsafe and nested blob names.
This join is now a filesystem trust boundary, but the supplied tests only cover the GitLab helpers. A small Azure-focused table test here would lock in traversal rejection and nested-path preservation for blob names too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/external/customtemplates/azure_blob.go` around lines 92 - 101, Add a table-driven unit test exercising Azure blob name handling to ensure safeJoinWithinDirectory and the downloadTemplate flow reject path-traversal names and preserve nested paths: create cases for an unsafe blob name like "../evil.txt" that should be rejected (assert error/log and no file created) and a nested blob name like "dir/sub/template.txt" that should be accepted and written under the configured download directory (assert resolved path is inside download dir and file content saved). Use the same Azure helper setup used by existing GitLab tests but target the functions safeJoinWithinDirectory and downloadTemplate (via the blob-processing code path that calls them) so the Azure branch in azure_blob.go is covered. Ensure assertions check both error/no-error and resulting filesystem paths, and add the table test to the package tests alongside the other helper tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/external/customtemplates/gitlab.go`:
- Around line 91-99: The per-project output path currently uses project.Path
(the repo slug) which can collide across namespaces; update the call sites that
compute projectOutputPath to use project.PathWithNamespace instead (i.e., pass
project.PathWithNamespace into safeProjectOutputPath and any directory-key
generation) so each project's directory is namespaced and cannot clobber another
project with the same slug; keep the existing containment validation
(safeProjectOutputPath) and MkdirAll usage unchanged, only replace project.Path
with project.PathWithNamespace where the per-project output directory is
constructed.
In `@pkg/protocols/headless/engine/page_actions_test.go`:
- Around line 285-306: The test TestFilesInputAndScreenshotShareLfaGate only
asserts protocolstate.IsLfaAllowed(opts) but never exercises the actual call
sites; update the test to also call page.isScreenshotPathAllowed (or the
exported equivalent used for screenshots) and to dispatch ActionFilesInput
through the page_actions dispatch so those code paths run under the same runtime
override; keep opts.AllowLocalFileAccess false, set
protocolstate.LfaAllowed.Set(executionId, true) as you already do, and assert
that both isScreenshotPathAllowed(...) and the ActionFilesInput dispatch behave
as allowed when the runtime override is present; ensure the existing cleanup of
protocolstate.LfaAllowed.Delete(executionId) remains.
---
Nitpick comments:
In `@pkg/external/customtemplates/azure_blob.go`:
- Around line 92-101: Add a table-driven unit test exercising Azure blob name
handling to ensure safeJoinWithinDirectory and the downloadTemplate flow reject
path-traversal names and preserve nested paths: create cases for an unsafe blob
name like "../evil.txt" that should be rejected (assert error/log and no file
created) and a nested blob name like "dir/sub/template.txt" that should be
accepted and written under the configured download directory (assert resolved
path is inside download dir and file content saved). Use the same Azure helper
setup used by existing GitLab tests but target the functions
safeJoinWithinDirectory and downloadTemplate (via the blob-processing code path
that calls them) so the Azure branch in azure_blob.go is covered. Ensure
assertions check both error/no-error and resulting filesystem paths, and add the
table test to the package tests alongside the other helper tests.
🪄 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: 2fab689a-3645-4095-bb3c-d5853d4bcebd
📒 Files selected for processing (16)
pkg/catalog/disk/find.gopkg/catalog/disk/find_test.gopkg/catalog/disk/path.gopkg/external/customtemplates/azure_blob.gopkg/external/customtemplates/gitlab.gopkg/external/customtemplates/gitlab_test.gopkg/external/customtemplates/s3.gopkg/external/customtemplates/s3_test.gopkg/installer/template.gopkg/installer/zipslip_unix_test.gopkg/protocols/headless/engine/page_actions.gopkg/protocols/headless/engine/page_actions_test.gopkg/reporting/exporters/markdown/markdown.gopkg/reporting/exporters/markdown/markdown_test.gopkg/utils/filepath/filepath.gopkg/utils/filepath/filepath_test.go
💤 Files with no reviewable changes (1)
- pkg/catalog/disk/path.go
Comment on lines
+285
to
+306
| func TestFilesInputAndScreenshotShareLfaGate(t *testing.T) { | ||
| executionId := t.Name() | ||
| t.Cleanup(func() { | ||
| protocolstate.LfaAllowed.Delete(executionId) | ||
| }) | ||
|
|
||
| opts := &types.Options{ExecutionId: executionId, AllowLocalFileAccess: false} | ||
|
|
||
| // Sanity: with no override, both gates must report deny. | ||
| require.False(t, protocolstate.IsLfaAllowed(opts), | ||
| "baseline IsLfaAllowed should be false when nothing is configured") | ||
|
|
||
| // Configure a runtime override via the LfaAllowed map without touching | ||
| // opts.AllowLocalFileAccess. | ||
| require.NoError(t, protocolstate.LfaAllowed.Set(executionId, true)) | ||
| require.True(t, protocolstate.IsLfaAllowed(opts), | ||
| "IsLfaAllowed must honour the LfaAllowed runtime override") | ||
|
|
||
| // The FilesInput dispatch in page_actions.go now calls IsLfaAllowed (the | ||
| // same predicate Screenshot uses), so the runtime override is respected | ||
| // without the caller having to also flip Options.AllowLocalFileAccess. | ||
| } |
Contributor
There was a problem hiding this comment.
Test name/intent doesn’t match what is asserted.
This test currently validates only protocolstate.IsLfaAllowed(opts). It never exercises page.isScreenshotPathAllowed or ActionFilesInput dispatch, so it can pass even if those call sites regress.
Suggested tightening
func TestFilesInputAndScreenshotShareLfaGate(t *testing.T) {
executionId := t.Name()
t.Cleanup(func() {
protocolstate.LfaAllowed.Delete(executionId)
})
opts := &types.Options{ExecutionId: executionId, AllowLocalFileAccess: false}
+ page := &Page{options: &Options{Options: opts}}
// Sanity: with no override, both gates must report deny.
require.False(t, protocolstate.IsLfaAllowed(opts),
"baseline IsLfaAllowed should be false when nothing is configured")
+ tmpDir := t.TempDir()
+ cwd := filepath.Join(tmpDir, "work")
+ outside := filepath.Join(tmpDir, "outside", "test.png")
+ require.NoError(t, os.MkdirAll(cwd, 0700))
+ require.NoError(t, os.MkdirAll(filepath.Dir(outside), 0700))
+ originalWd, err := os.Getwd()
+ require.NoError(t, err)
+ require.NoError(t, os.Chdir(cwd))
+ t.Cleanup(func() { require.NoError(t, os.Chdir(originalWd)) })
+
+ require.ErrorIs(t, page.isScreenshotPathAllowed(outside), ErrLFAccessDenied)
+
// Configure a runtime override via the LfaAllowed map without touching
// opts.AllowLocalFileAccess.
require.NoError(t, protocolstate.LfaAllowed.Set(executionId, true))
require.True(t, protocolstate.IsLfaAllowed(opts),
"IsLfaAllowed must honour the LfaAllowed runtime override")
+
+ require.NoError(t, page.isScreenshotPathAllowed(outside))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/protocols/headless/engine/page_actions_test.go` around lines 285 - 306,
The test TestFilesInputAndScreenshotShareLfaGate only asserts
protocolstate.IsLfaAllowed(opts) but never exercises the actual call sites;
update the test to also call page.isScreenshotPathAllowed (or the exported
equivalent used for screenshots) and to dispatch ActionFilesInput through the
page_actions dispatch so those code paths run under the same runtime override;
keep opts.AllowLocalFileAccess false, set
protocolstate.LfaAllowed.Set(executionId, true) as you already do, and assert
that both isScreenshotPathAllowed(...) and the ActionFilesInput dispatch behave
as allowed when the runtime override is present; ensure the existing cleanup of
protocolstate.LfaAllowed.Delete(executionId) remains.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/protocols/headless/engine/page_actions_test.go (1)
284-329:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest intent still overstates ActionFilesInput coverage.
This test validates
protocolstate.IsLfaAllowedandpage.isScreenshotPathAllowed, but it still does not execute theExecuteActionsbranch forActionFilesInput, so a dispatch regression there can pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/protocols/headless/engine/page_actions_test.go` around lines 284 - 329, The test TestFilesInputAndScreenshotShareLfaGate currently checks protocolstate.IsLfaAllowed and page.isScreenshotPathAllowed but never exercises the ExecuteActions dispatch for ActionFilesInput; update the test to also call ExecuteActions (the function that handles ActionFilesInput in page_actions.go) with an ActionFilesInput targeting outsideTarget and assert it errors with ErrLFAccessDenied before setting protocolstate.LfaAllowed.Set(executionId, true), then assert ExecuteActions succeeds after the runtime override so the ExecuteActions branch is covered and any regression in ActionFilesInput gating is detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/protocols/common/protocolstate/state.go`:
- Around line 58-71: The Dialers initialization and Init refresh path are
inconsistent: initDialers() must set Dialers.RestrictLocalNetworkAccess and
build Dialers.NetworkPolicy based on options.RestrictLocalNetworkAccess so
IsRestrictLocalNetworkAccess() reflects the option; likewise, in the Init()
refresh branch (where GetDialersWithId(options.ExecutionId) returns
existingDialers) update existingDialers.RestrictLocalNetworkAccess and also
recreate or update existingDialers.NetworkPolicy to reflect the new
RestrictLocalNetworkAccess value (and then call SetLfaAllowed(options) as
before) so deny-list enforcement matches the refreshed setting.
---
Duplicate comments:
In `@pkg/protocols/headless/engine/page_actions_test.go`:
- Around line 284-329: The test TestFilesInputAndScreenshotShareLfaGate
currently checks protocolstate.IsLfaAllowed and page.isScreenshotPathAllowed but
never exercises the ExecuteActions dispatch for ActionFilesInput; update the
test to also call ExecuteActions (the function that handles ActionFilesInput in
page_actions.go) with an ActionFilesInput targeting outsideTarget and assert it
errors with ErrLFAccessDenied before setting
protocolstate.LfaAllowed.Set(executionId, true), then assert ExecuteActions
succeeds after the runtime override so the ExecuteActions branch is covered and
any regression in ActionFilesInput gating is detected.
🪄 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: 7c16708b-8d02-4423-80a9-502777034170
📒 Files selected for processing (8)
pkg/external/customtemplates/azure_blob_test.gopkg/external/customtemplates/gitlab.gopkg/installer/zipslip_unix_test.gopkg/protocols/common/protocolstate/state.gopkg/protocols/headless/engine/page_actions.gopkg/protocols/headless/engine/page_actions_test.gopkg/types/types.gopkg/types/types_test.go
Comment on lines
+58
to
71
| if existingDialers := GetDialersWithId(options.ExecutionId); existingDialers != nil { | ||
| // Dialers already exist for this ExecutionId. Refresh the LFA / | ||
| // network-policy state derived from options so that a second | ||
| // Init call with different options (e.g. flipping | ||
| // AllowLocalFileAccess) is reflected in IsLfaAllowed and the | ||
| // per-execution dialer state. Without this refresh the second | ||
| // caller silently keeps the first caller's settings, which is a | ||
| // footgun for tests and SDK callers that share an execution id. | ||
| existingDialers.Lock() | ||
| existingDialers.LocalFileAccessAllowed = options.AllowLocalFileAccess | ||
| existingDialers.RestrictLocalNetworkAccess = options.RestrictLocalNetworkAccess | ||
| existingDialers.Unlock() | ||
| SetLfaAllowed(options) | ||
| return nil |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if RestrictLocalNetworkAccess is read from the Dialers struct elsewhere
echo "=== Searching for reads of RestrictLocalNetworkAccess from Dialers ==="
rg -n --type=go 'RestrictLocalNetworkAccess' -C 3
echo ""
echo "=== Checking Dialers struct definition ==="
ast-grep --pattern 'type Dialers struct {
$$$
}'Repository: projectdiscovery/nuclei
Length of output: 7985
Fix RestrictLocalNetworkAccess initialization/refresh inconsistency
initDialers()createsDialerswithout settingRestrictLocalNetworkAccess, soIsRestrictLocalNetworkAccess()(readsdialers.RestrictLocalNetworkAccess) will returnfalseeven whenoptions.RestrictLocalNetworkAccessistrue.- The
Init()refresh path updatesexistingDialers.RestrictLocalNetworkAccess, but it does not recreateexistingDialers.NetworkPolicy, so the deny-list enforcement won’t reflect a changedRestrictLocalNetworkAccesson subsequentInitcalls for the sameExecutionId.
Add consistent initialization
dialersInstance := &Dialers{
Fastdialer: dialer,
NetworkPolicy: networkPolicy,
HTTPClientPool: httpClientPool,
LocalFileAccessAllowed: options.AllowLocalFileAccess,
+ RestrictLocalNetworkAccess: options.RestrictLocalNetworkAccess,
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/protocols/common/protocolstate/state.go` around lines 58 - 71, The
Dialers initialization and Init refresh path are inconsistent: initDialers()
must set Dialers.RestrictLocalNetworkAccess and build Dialers.NetworkPolicy
based on options.RestrictLocalNetworkAccess so IsRestrictLocalNetworkAccess()
reflects the option; likewise, in the Init() refresh branch (where
GetDialersWithId(options.ExecutionId) returns existingDialers) update
existingDialers.RestrictLocalNetworkAccess and also recreate or update
existingDialers.NetworkPolicy to reflect the new RestrictLocalNetworkAccess
value (and then call SetLfaAllowed(options) as before) so deny-list enforcement
matches the refreshed setting.
ehsandeep
deleted the
dwisiswant0/refactor/use-path-aware-filesystem-containment-checks
branch
June 4, 2026 15:33
4 tasks
4 tasks
This was referenced Jul 26, 2026
9 tasks
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
3 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
Several filesystem trust boundaries relied on
lexical prefix checks to decide whether a path
fell under an allowed directory. That let sibling
paths be treated as if they were children of
trusted directories.
Replace those checks with canonical path
containment checks for helper payload loading,
template archive extraction, custom template
metadata, template path classification, and
headless screenshot output validation.
Proof
Checklist
Summary by CodeRabbit