Conversation
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:
WalkthroughThis PR centralizes filesystem, network, and DSN enforcement, adds Landlock and Docker-backed sandboxing, migrates JavaScript protocol libraries to policy-aware access paths, introduces allowed-path and sandbox-control options, and expands unit, integration, and platform-specific tests. ChangesSandbox and Allowlist Enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Template
participant JSProtocol
participant protocolstate
participant NetworkPolicy
participant Fastdialer
participant Sandbox
Template->>JSProtocol: Request file or network operation
JSProtocol->>protocolstate: ReadFileAllowed or DialAllowedWithExecutionID
protocolstate->>NetworkPolicy: Validate execution-scoped policy
alt Access allowed
protocolstate->>Fastdialer: Perform network dial
protocolstate-->>JSProtocol: Return permitted result
else Access denied
protocolstate-->>JSProtocol: Return access error
end
Note over Sandbox: Landlock and Docker confinement enforce sandbox boundaries
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mzack9999
marked this pull request as draft
July 3, 2026 00:22
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/protocols/common/protocolstate/headless.go (1)
98-121: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
isValidHostshould fail closed when dialers are missing.
ValidateNFailRequeststill treats a missingdialers.Get(...)result as allowed, so headless request validation can pass through without network-policy enforcement. Align this path withIsHostAllowedand returnfalsewhen dialers are unavailable.🤖 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/headless.go` around lines 98 - 121, The isValidHost path is currently allowing requests to pass when dialers.Get(options.ExecutionId) is missing, which leaves ValidateNFailRequest without network-policy enforcement. Update isValidHost in protocolstate/headless.go to fail closed like IsHostAllowed by returning false when the dialers lookup does not succeed, and keep the existing NetworkPolicy validation flow for the valid dialers case.pkg/js/libs/pop3/pop3.go (1)
43-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd the host-policy guard before dialing
DialAllowedWithExecutionIDonly checksexecutionIdand the execution-scoped fastdialer; it does not enforceIsHostAllowed, so this path bypasses the network-policy guard used elsewhere.- The local
GetDialersWithIdnil-check is redundant and can be removed.🤖 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/js/libs/pop3/pop3.go` around lines 43 - 58, The `isPoP3` path is bypassing the host-policy check because it dials before enforcing the network guard. Remove the redundant `protocolstate.GetDialersWithId` nil-check, and add an `IsHostAllowed` guard in `isPoP3` before calling `protocolstate.DialAllowedWithExecutionID` so the host is validated consistently with other network paths.
🧹 Nitpick comments (13)
pkg/js/libs/smb/smbghost.go (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant dialer lookup before
DialAllowedWithExecutionID.Same pattern as mysql.go:
dialer := protocolstate.GetDialersWithId(executionId)is fetched and nil-checked but never used, sinceDialAllowedWithExecutionIDperforms an identical internal check before dialing. Unlike the other cases, this file does correctly retain theIsHostAllowedcheck above it, so this is purely a dead-code cleanup.♻️ Proposed cleanup
addr := net.JoinHostPort(host, strconv.Itoa(port)) - dialer := protocolstate.GetDialersWithId(executionId) - if dialer == nil { - return false, fmt.Errorf("dialers not initialized for %s", executionId) - } conn, err := protocolstate.DialAllowedWithExecutionID(ctx, executionId, "tcp", addr)🤖 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/js/libs/smb/smbghost.go` around lines 41 - 45, Remove the redundant pre-check in smbghost.go by deleting the unused protocolstate.GetDialersWithId(executionId) lookup and its nil return before calling protocolstate.DialAllowedWithExecutionID in the SMB dial path; keep the existing protocolstate.IsHostAllowed validation, and let DialAllowedWithExecutionID handle the dialer initialization check itself.pkg/js/libs/mysql/mysql.go (1)
49-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant dialer lookup before
DialAllowedWithExecutionID.
dialer := protocolstate.GetDialersWithId(executionId)and the nil-check are fetched but the returneddialervalue is never used for the actual connection —DialAllowedWithExecutionIDperforms the exact same lookup and nil-check internally (perpkg/protocols/common/protocolstate/dial.go:35-44) before dialing. This duplicates the check with a slightly different error string ("dialers not initialized for %s" vs "protocolstate: dialers not initialized for %q"), which is confusing for future maintainers.♻️ Proposed cleanup (apply to both isMySQL and fingerprintMySQL)
func isMySQL(ctx context.Context, executionId string, host string, port int) (bool, error) { if !protocolstate.IsHostAllowed(executionId, host) { // host is not valid according to network policy return false, protocolstate.ErrHostDenied.Msgf(host) } - dialer := protocolstate.GetDialersWithId(executionId) - if dialer == nil { - return false, fmt.Errorf("dialers not initialized for %s", executionId) - } - conn, err := protocolstate.DialAllowedWithExecutionID(ctx, executionId, "tcp", net.JoinHostPort(host, fmt.Sprintf("%d", port)))Also applies to: 148-153
🤖 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/js/libs/mysql/mysql.go` around lines 49 - 54, Remove the redundant pre-check in isMySQL and fingerprintMySQL by eliminating the direct protocolstate.GetDialersWithId lookup and nil branch, since protocolstate.DialAllowedWithExecutionID already performs that validation internally. Keep the connection attempt logic in these MySQL helpers focused on calling DialAllowedWithExecutionID and handling its error, so the error path and message come from the shared protocolstate behavior rather than a duplicated local check.pkg/js/libs/guard/fastdialer_guard_test.go (1)
17-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGuard exclusion and offender detection both rely on fragile substring matching.
Two related weaknesses:
strings.Contains(path, string(filepath.Join("guard")))(line 20) —filepath.Join("guard")is a no-op that just returns"guard", so this skips any path containing the substring "guard" anywhere, not just the intendedguard/directory (e.g. a hypotheticalvanguardpackage would also be silently excluded from the check).strings.Contains(content, "Fastdialer.Dial")— this only catches the literal selector expression. Aliasing (fd := dialer.Fastdialer; fd.Dial(...)) or splitting the expression across lines trivially bypasses this guard, since this is exactly the kind of raw-syscall/dial bypass this CI guard is meant to prevent.Since this test is a security guard rail meant to enforce the centralized dialing contract, consider tightening the directory check (e.g. compare
filepath.Base(filepath.Dir(path)) == "guard") and/or using an AST-based check (go/ast, orast-grep) to detect selector expressions onFastdialer, rather than plain text search.🛡️ Suggested fix for the directory-exclusion bug
- if strings.Contains(path, string(filepath.Join("guard"))) { + if filepath.Base(filepath.Dir(path)) == "guard" { return nil }🤖 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/js/libs/guard/fastdialer_guard_test.go` around lines 17 - 30, The guard in fastdialer_guard_test.go uses fragile substring matching for both directory exclusion and offender detection. Update the path filter in the file-walk logic so it only skips the intended guard directory via explicit path component checks, and replace the raw strings.Contains checks in the offender scan with a more robust AST-based detection in the test helper that identifies Fastdialer selector calls reliably. Keep the fix centered around the existing walk/filter logic and the Fastdialer.Dial / Fastdialer.DialTLS detection paths.pkg/js/libs/oracle/oracle_test.go (1)
73-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest name implies
--allowed-pathscoverage but doesn't exercise it.
TestSandboxDSNAllowsTraceFileWithinAllowedPathsWithLFAplacestraceFileinsidetemplatesDir, which is always permitted regardless of the LFA flag (perNormalizePath's allowlist logic). This is functionally identical to the "WithoutLFA" test above it (line 50) except for the LFA flag value — neither actually exercises theAllowedFileRoots/--allowed-pathsallowlist mechanism this PR introduces. Consider adding a path outside bothtemplatesDirand LFA scope that is only permitted via an explicitAllowedPathsentry, to genuinely validate that boundary.🤖 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/js/libs/oracle/oracle_test.go` around lines 73 - 91, The test TestSandboxDSNAllowsTraceFileWithinAllowedPathsWithLFA does not actually cover the new allowed-paths behavior because it only uses a trace file under templatesDir, which is already permitted by NormalizePath. Update this test to use a trace file path outside both templatesDir and the LFA scope, then configure the relevant AllowedPaths/AllowedFileRoots entry so protocolstate.SanitizeOracleDSN and go_ora.ParseConfig only succeed because of the explicit allowlist. Keep the existing assertions, but make the setup exercise the boundary that the new allowlist logic in protocolstate and the Oracle DSN sandboxing is meant to protect.pkg/js/libs/smb/smb_private.go (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame redundant nil-dialer pre-check pattern.
Consistent with the same duplication flagged in
rdp.goandsmb.go—dialer(Line 25-28) is unused after the nil check sinceDialAllowedWithExecutionIDre-checks internally.🤖 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/js/libs/smb/smb_private.go` around lines 25 - 33, Remove the redundant nil-dialer pre-check in smbPrivate dial logic: the local dialer lookup in smbPrivate is only used to guard before calling DialAllowedWithExecutionID, but that helper already performs the same initialization check internally. Update smbPrivate to rely on DialAllowedWithExecutionID for validation and keep only the actual connection attempt and error handling, matching the cleanup done in the related rdp and smb call sites.pkg/js/libs/rdp/rdp.go (1)
49-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDial migration correct; stale nil-dialer pre-checks are now redundant.
DialAllowedWithExecutionIDalready performs theGetDialersWithId(...) == nilcheck internally, so the localdialer := protocolstate.GetDialersWithId(executionId)blocks inisRDP(lines 49-52),checkRDPAuth(lines 108-111), andcheckRDPEncryption(lines 196-199) are now dead code —dialeris never used afterward.Apply the same cleanup to `checkRDPAuth` and `checkRDPEncryption`.♻️ Example cleanup for isRDP
func isRDP(ctx context.Context, executionId string, host string, port int) (IsRDPResponse, error) { resp := IsRDPResponse{} - - dialer := protocolstate.GetDialersWithId(executionId) - if dialer == nil { - return IsRDPResponse{}, fmt.Errorf("dialers not initialized for %s", executionId) - } - timeout := 5 * time.Second conn, err := protocolstate.DialAllowedWithExecutionID(ctx, executionId, "tcp", fmt.Sprintf("%s:%d", host, port))Also applies to: 108-116, 196-199
🤖 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/js/libs/rdp/rdp.go` around lines 49 - 58, Remove the redundant nil-dialer pre-checks in isRDP, checkRDPAuth, and checkRDPEncryption: protocolstate.DialAllowedWithExecutionID already handles protocolstate.GetDialersWithId(executionId) == nil internally, so the local dialer := protocolstate.GetDialersWithId(executionId) assignments and their immediate nil-return branches are dead code. Keep the actual dial/connection logic in these functions and rely on DialAllowedWithExecutionID for dialer initialization handling.pkg/js/libs/guard/jslibs_guard_test.go (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprecise directory exclusion via substring match.
strings.Contains(path, string(filepath.Join("guard")))matches any path containing the substring "guard" (e.g. a futureguardianpackage or file), not just the intendedguarddirectory. Prefer matching path components explicitly.♻️ Suggested fix
- if strings.Contains(path, string(filepath.Join("guard"))) { + if strings.Contains(filepath.ToSlash(path), "/guard/") { return nil }🤖 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/js/libs/guard/jslibs_guard_test.go` around lines 31 - 33, The path filter in the test helper is too broad because the strings.Contains check on guard can match unrelated paths like guardian; update the exclusion logic in the test’s path-matching helper to match the guard path component explicitly instead of using a substring search. Use the existing path handling around filepath.Join and strings.Contains in jslibs_guard_test.go to tighten the condition so only the intended directory is excluded.pkg/js/libs/smb/smb.go (1)
49-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame redundant nil-dialer pre-check as elsewhere.
dialer := protocolstate.GetDialersWithId(executionId)at Line 49-52 and Line 130-133 is now dead validation —DialAllowedWithExecutionIDperforms the identical nil check internally anddialerisn't used afterward. Same pattern flagged inpkg/js/libs/rdp/rdp.go.Also applies to: 130-138
🤖 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/js/libs/smb/smb.go` around lines 49 - 56, Remove the redundant nil-dialer pre-check in smb.go by deleting the unused GetDialersWithId/executionId validation before dialing, since DialAllowedWithExecutionID already performs the same check internally. Update both SMB connection paths (the logic around dialSMBInfo and the matching block near the later SMB helper) so they rely on DialAllowedWithExecutionID for validation and error handling, keeping only the address construction and dial closure setup.pkg/js/libs/krbforge/krbforge.go (1)
200-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for relative output path +
-lfaenabled.The relative-path-under-
TempDir()branch (Lines 206-208) isn't exercised bykrbforge_test.go— the LFA-enabled test uses an already-absolute cwd path. Consider adding a case for a bare relativeOutputFilewith LFA enabled to lock in this redirection 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/js/libs/krbforge/krbforge.go` around lines 200 - 213, The relative output path handling in normalizeOutputFile is missing test coverage for the protocolstate.IsLfaAllowed branch. Add a krbforge_test.go case that calls normalizeOutputFile with a bare relative OutputFile while LFA is enabled, and assert it is redirected under os.TempDir() before normalization, using normalizeOutputFile and protocolstate.IsLfaAllowed as the key symbols.pkg/protocols/common/protocolstate/allowlist.go (1)
74-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
isPathAllowedrecomputesAllowedFileRoots(withEvalSymlinkssyscalls) on every call.Every
NormalizePath/ReadFileAllowed/DSN-sanitize invocation re-runsAllowedFileRoots, which doesfilepath.Abs+filepath.EvalSymlinksfor template/config/temp/store dirs (and cwd/AllowedPaths when LFA is on) each time. For JS templates doing repeated file reads, this adds avoidable stat/readlink syscalls per call.Consider memoizing the computed roots per
options/execution (e.g., cache on first access, invalidate only whenAllowedPaths/LFA state changes) rather than recomputing on the hot path.🤖 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/allowlist.go` around lines 74 - 76, The hot path in isPathAllowed is recomputing AllowedFileRoots on every call, which repeats expensive filepath.Abs and filepath.EvalSymlinks work. Update isPathAllowed to use a memoized/cached set of allowed roots tied to the current options or execution context, and only refresh the cache when AllowedPaths or LFA-related state changes. Keep the existing IsPathWithinAnyDirectory check, but avoid calling AllowedFileRoots repeatedly from NormalizePath/ReadFileAllowed/DSN-sanitize flows.pkg/protocols/code/sandbox_exec_linux_test.go (1)
17-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for environment isolation and the bwrap-unavailable fallback.
Consider adding:
- A test asserting a host-only env var (set via
t.Setenvbeforetestutils.Init) is NOT visible to the sandboxed script'senvoutput - this would have caught the missing--clearenvissue.- A test that forces
shouldFallbackFromBubblewrap(e.g. by stubbingbwrapto fail with a namespace error) to confirm the fallback behavior is intentional and to lock in expected behavior once the silent-fallback issue above is addressed.🤖 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/code/sandbox_exec_linux_test.go` around lines 17 - 101, Add test coverage in the existing sandbox execution tests to lock in environment isolation and the bubblewrap fallback behavior. In TestCodeProtocolBubblewrapExecution or a new sibling test, set a host-only variable with t.Setenv before testutils.Init and assert it does not appear in the script’s env output when run through Request.ExecuteWithResults. Also add a targeted test around shouldFallbackFromBubblewrap that stubs the bwrap failure path (namespace error) and verifies the expected fallback is exercised, using the same Request and bubblewrapFunctional setup patterns already present.pkg/protocols/common/protocolstate/dsn.go (1)
27-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRedundant/possibly-divergent identity params in
SanitizeOracleDSN.The function takes both an
executionIDstring and anoptions *types.Options(with its ownExecutionId).executionIDis only used whenoptions == nil; otherwiseoptions.ExecutionIdsilently drivesNormalizePath's execution-scoped checks. If a caller ever passes mismatched values, the wrong execution's allow-state is consulted. SinceExecutionIdgates filesystem permission scope, this invariant should be enforced rather than assumed.🔧 Proposed fix to reconcile the two identifiers
func SanitizeOracleDSN(executionID, dsn string, options *types.Options) (string, error) { parsed, err := url.Parse(dsn) if err != nil { return "", err } if options == nil { options = &types.Options{ExecutionId: executionID} + } else if options.ExecutionId != executionID { + cloned := *options + cloned.ExecutionId = executionID + options = &cloned }🤖 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/dsn.go` around lines 27 - 34, SanitizeOracleDSN currently accepts both executionID and options.ExecutionId, which can diverge and cause NormalizePath to use the wrong execution scope. Update SanitizeOracleDSN to enforce a single source of truth by validating that executionID matches options.ExecutionId when options is provided, or by deriving options consistently from executionID before calling NormalizePath. Use the SanitizeOracleDSN and NormalizePath symbols to keep execution-scoped path checks aligned.pkg/protocols/common/protocolstate/fsbroker.go (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
OpenFileAllowed.
ReadFileAllowedandWriteFileAllowedboth have allow/reject tests infsbroker_test.go, butOpenFileAllowedhas none. Given this is a security-boundary function, add matching coverage.🤖 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/fsbroker.go` around lines 18 - 25, Add missing security-boundary tests for OpenFileAllowed in fsbroker_test.go, matching the existing allow/reject coverage used by ReadFileAllowed and WriteFileAllowed. Create tests that exercise OpenFileAllowed with allowed and disallowed paths via NormalizePath behavior, and verify it successfully opens permitted files while rejecting forbidden ones. Use the OpenFileAllowed symbol (and its dependency on NormalizePath) to keep the new coverage aligned with the other filesystem broker 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/js/compiler/compiler_test.go`:
- Around line 49-55: The outsideModulePath helper is using os.Getenv("HOME"),
which can be empty on some environments and cause the generated path to resolve
incorrectly. Update outsideModulePath to use os.UserHomeDir() instead, handle
its error with the existing testing assertions, and keep the rest of the path
construction and writeModuleFile call unchanged so the “outside” path is always
truly outside the templates dir.
In `@pkg/js/libs/guard/jslibs_guard_test.go`:
- Around line 13-20: The guard map includes an unreachable selector for
exec.Command because the matcher uses call selectors from imported package
names, so update forbiddenSelectors in jslibs_guard_test to use the actual
selector key exec.Command and leave net.Dialer only if you also add type-aware
detection for constructor/type usage. Verify the test logic in the guard check
path still blocks direct calls by matching against the selector names produced
from imports, and adjust any helper that builds the selector string if needed.
In `@pkg/js/libs/krbforge/krbforge_test.go`:
- Around line 16-19: Update the outsideSandboxPath helper to use
os.UserHomeDir() instead of os.Getenv("HOME"), since relying on HOME can produce
a relative path on Windows and break the outside-sandbox assertions; keep the
existing filepath.Join logic and make sure the helper still returns a path
rooted in the user home directory for the krbforge tests.
In `@pkg/js/libs/postgres/postgres.go`:
- Line 53: The address formatting in the connection setup is not IPv6-safe
because the current host and port string construction can break for literals
like ::1. Update the call in the postgres connection flow that uses
protocolstate.DialAllowedWithExecutionID to build the target address with
net.JoinHostPort, matching the existing patterns used elsewhere in this file for
consistency and correct IPv6 handling.
In `@pkg/js/libs/redis/redis.go`:
- Line 159: The Redis dial address is built with string formatting that is not
IPv6-safe. Update the `protocolstate.DialAllowedWithExecutionID` call in
`redis.go` to use `net.JoinHostPort` with the host and stringified port, and add
the needed `net` and `strconv` imports. Keep the change localized around the
Redis connection setup so it matches the existing address-building pattern used
elsewhere.
In `@pkg/protocols/code/sandbox_exec_linux.go`:
- Around line 44-66: The tryEvalSandboxed fallback currently downgrades
namespace/permission failures from evalBubblewrap into a silent unsandboxed run,
which hides the security boundary change. Update tryEvalSandboxed and the caller
path around evalCode/gozero.Eval so shouldFallbackFromBubblewrap no longer
returns a nil error for sandbox-creation failures; instead surface a hard error
(or a clearly propagated failure) when bubblewrap cannot create namespaces,
keeping only the explicit DisableSandbox path as a true opt-out. Use the
existing symbols tryEvalSandboxed, shouldFallbackFromBubblewrap, evalBubblewrap,
and evalCode to locate the control flow.
- Around line 100-136: Add `--clearenv` to the bubblewrap argument list built in
`sandbox_exec_linux.go` so the sandbox does not inherit the parent process
environment. Update the `bwrapArgs` construction in the sandbox execution flow
to clear inherited variables before applying the explicit `--setenv` entries
from `mergeVariables`, while preserving the existing `PATH` fallback and other
options in the `request.options.Interactsh` / `protocolstate.AllowedFileRoots`
path.
In `@pkg/protocols/common/protocolstate/file.go`:
- Around line 67-72: `AllowedPaths` is not being applied to execution-scoped
path validation because `AllowedFileRoots` only sees the execution-id path
options. Update the path-check flow in `file.go` so the callers like
`NormalizePathWithExecutionId`, `fs.ReadFile`, and
`krbforge.normalizeOutputFile` pass the full options through (or look up stored
allowed roots by execution ID) instead of only `ExecutionId`, and ensure
`isPathAllowed`/`AllowedFileRoots` use those propagated roots when evaluating
`options.AllowedPaths`.
In `@pkg/protocols/common/protocolstate/fsbroker_test.go`:
- Around line 26-38: The negative-path test uses os.Getenv("HOME") to build the
“outside” path, which is brittle and can resolve incorrectly when HOME is unset
or on non-Unix systems. Update TestReadFileAllowedRejectsOutsideTemplates (and
the other affected test) to create the disallowed file in a separate t.TempDir()
outside the templatesDir, then use that absolute path for ReadFileAllowed. Keep
the existing helpers like restoreTemplatesDir and protocolstate.ReadFileAllowed,
but remove the HOME-based path construction.
In `@pkg/protocols/common/sandbox/sandbox_linux.go`:
- Around line 11-13: platformSupported() currently hardcodes Linux as supported,
which lets Supported() and BestEffort() succeed even when the kernel cannot
actually enforce Landlock. Update platformSupported() in the sandbox_linux.go
path to perform a runtime Landlock capability/probe check before returning true,
and have Apply() treat unsupported kernels as not sandboxed rather than
succeeding silently. Use the existing platformSupported(), Supported(),
BestEffort(), and Apply() flow to locate and wire the check.
In `@pkg/protocols/common/sandbox/sandbox.go`:
- Around line 28-48: Apply currently uses sync.Once in a way that locks in
failures, so a bad first Config leaves appliedErr permanently set and blocks
later valid calls. Update Apply to only mark the sandbox as applied after a
successful applyPlatform call, and allow retries when the previous attempt
returned ErrNoAllowedRoots or another error. Use the existing Apply function,
appliedOnce, appliedErr, and the Supported/applyPlatform paths to restructure
the state handling so only the first successful apply wins.
---
Outside diff comments:
In `@pkg/js/libs/pop3/pop3.go`:
- Around line 43-58: The `isPoP3` path is bypassing the host-policy check
because it dials before enforcing the network guard. Remove the redundant
`protocolstate.GetDialersWithId` nil-check, and add an `IsHostAllowed` guard in
`isPoP3` before calling `protocolstate.DialAllowedWithExecutionID` so the host
is validated consistently with other network paths.
In `@pkg/protocols/common/protocolstate/headless.go`:
- Around line 98-121: The isValidHost path is currently allowing requests to
pass when dialers.Get(options.ExecutionId) is missing, which leaves
ValidateNFailRequest without network-policy enforcement. Update isValidHost in
protocolstate/headless.go to fail closed like IsHostAllowed by returning false
when the dialers lookup does not succeed, and keep the existing NetworkPolicy
validation flow for the valid dialers case.
---
Nitpick comments:
In `@pkg/js/libs/guard/fastdialer_guard_test.go`:
- Around line 17-30: The guard in fastdialer_guard_test.go uses fragile
substring matching for both directory exclusion and offender detection. Update
the path filter in the file-walk logic so it only skips the intended guard
directory via explicit path component checks, and replace the raw
strings.Contains checks in the offender scan with a more robust AST-based
detection in the test helper that identifies Fastdialer selector calls reliably.
Keep the fix centered around the existing walk/filter logic and the
Fastdialer.Dial / Fastdialer.DialTLS detection paths.
In `@pkg/js/libs/guard/jslibs_guard_test.go`:
- Around line 31-33: The path filter in the test helper is too broad because the
strings.Contains check on guard can match unrelated paths like guardian; update
the exclusion logic in the test’s path-matching helper to match the guard path
component explicitly instead of using a substring search. Use the existing path
handling around filepath.Join and strings.Contains in jslibs_guard_test.go to
tighten the condition so only the intended directory is excluded.
In `@pkg/js/libs/krbforge/krbforge.go`:
- Around line 200-213: The relative output path handling in normalizeOutputFile
is missing test coverage for the protocolstate.IsLfaAllowed branch. Add a
krbforge_test.go case that calls normalizeOutputFile with a bare relative
OutputFile while LFA is enabled, and assert it is redirected under os.TempDir()
before normalization, using normalizeOutputFile and protocolstate.IsLfaAllowed
as the key symbols.
In `@pkg/js/libs/mysql/mysql.go`:
- Around line 49-54: Remove the redundant pre-check in isMySQL and
fingerprintMySQL by eliminating the direct protocolstate.GetDialersWithId lookup
and nil branch, since protocolstate.DialAllowedWithExecutionID already performs
that validation internally. Keep the connection attempt logic in these MySQL
helpers focused on calling DialAllowedWithExecutionID and handling its error, so
the error path and message come from the shared protocolstate behavior rather
than a duplicated local check.
In `@pkg/js/libs/oracle/oracle_test.go`:
- Around line 73-91: The test
TestSandboxDSNAllowsTraceFileWithinAllowedPathsWithLFA does not actually cover
the new allowed-paths behavior because it only uses a trace file under
templatesDir, which is already permitted by NormalizePath. Update this test to
use a trace file path outside both templatesDir and the LFA scope, then
configure the relevant AllowedPaths/AllowedFileRoots entry so
protocolstate.SanitizeOracleDSN and go_ora.ParseConfig only succeed because of
the explicit allowlist. Keep the existing assertions, but make the setup
exercise the boundary that the new allowlist logic in protocolstate and the
Oracle DSN sandboxing is meant to protect.
In `@pkg/js/libs/rdp/rdp.go`:
- Around line 49-58: Remove the redundant nil-dialer pre-checks in isRDP,
checkRDPAuth, and checkRDPEncryption: protocolstate.DialAllowedWithExecutionID
already handles protocolstate.GetDialersWithId(executionId) == nil internally,
so the local dialer := protocolstate.GetDialersWithId(executionId) assignments
and their immediate nil-return branches are dead code. Keep the actual
dial/connection logic in these functions and rely on DialAllowedWithExecutionID
for dialer initialization handling.
In `@pkg/js/libs/smb/smb_private.go`:
- Around line 25-33: Remove the redundant nil-dialer pre-check in smbPrivate
dial logic: the local dialer lookup in smbPrivate is only used to guard before
calling DialAllowedWithExecutionID, but that helper already performs the same
initialization check internally. Update smbPrivate to rely on
DialAllowedWithExecutionID for validation and keep only the actual connection
attempt and error handling, matching the cleanup done in the related rdp and smb
call sites.
In `@pkg/js/libs/smb/smb.go`:
- Around line 49-56: Remove the redundant nil-dialer pre-check in smb.go by
deleting the unused GetDialersWithId/executionId validation before dialing,
since DialAllowedWithExecutionID already performs the same check internally.
Update both SMB connection paths (the logic around dialSMBInfo and the matching
block near the later SMB helper) so they rely on DialAllowedWithExecutionID for
validation and error handling, keeping only the address construction and dial
closure setup.
In `@pkg/js/libs/smb/smbghost.go`:
- Around line 41-45: Remove the redundant pre-check in smbghost.go by deleting
the unused protocolstate.GetDialersWithId(executionId) lookup and its nil return
before calling protocolstate.DialAllowedWithExecutionID in the SMB dial path;
keep the existing protocolstate.IsHostAllowed validation, and let
DialAllowedWithExecutionID handle the dialer initialization check itself.
In `@pkg/protocols/code/sandbox_exec_linux_test.go`:
- Around line 17-101: Add test coverage in the existing sandbox execution tests
to lock in environment isolation and the bubblewrap fallback behavior. In
TestCodeProtocolBubblewrapExecution or a new sibling test, set a host-only
variable with t.Setenv before testutils.Init and assert it does not appear in
the script’s env output when run through Request.ExecuteWithResults. Also add a
targeted test around shouldFallbackFromBubblewrap that stubs the bwrap failure
path (namespace error) and verifies the expected fallback is exercised, using
the same Request and bubblewrapFunctional setup patterns already present.
In `@pkg/protocols/common/protocolstate/allowlist.go`:
- Around line 74-76: The hot path in isPathAllowed is recomputing
AllowedFileRoots on every call, which repeats expensive filepath.Abs and
filepath.EvalSymlinks work. Update isPathAllowed to use a memoized/cached set of
allowed roots tied to the current options or execution context, and only refresh
the cache when AllowedPaths or LFA-related state changes. Keep the existing
IsPathWithinAnyDirectory check, but avoid calling AllowedFileRoots repeatedly
from NormalizePath/ReadFileAllowed/DSN-sanitize flows.
In `@pkg/protocols/common/protocolstate/dsn.go`:
- Around line 27-34: SanitizeOracleDSN currently accepts both executionID and
options.ExecutionId, which can diverge and cause NormalizePath to use the wrong
execution scope. Update SanitizeOracleDSN to enforce a single source of truth by
validating that executionID matches options.ExecutionId when options is
provided, or by deriving options consistently from executionID before calling
NormalizePath. Use the SanitizeOracleDSN and NormalizePath symbols to keep
execution-scoped path checks aligned.
In `@pkg/protocols/common/protocolstate/fsbroker.go`:
- Around line 18-25: Add missing security-boundary tests for OpenFileAllowed in
fsbroker_test.go, matching the existing allow/reject coverage used by
ReadFileAllowed and WriteFileAllowed. Create tests that exercise OpenFileAllowed
with allowed and disallowed paths via NormalizePath behavior, and verify it
successfully opens permitted files while rejecting forbidden ones. Use the
OpenFileAllowed symbol (and its dependency on NormalizePath) to keep the new
coverage aligned with the other filesystem broker 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: 4c18c510-36ca-4fad-a06f-3380504c9e46
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.suminternal/tests/integration/testdata/protocols/javascript/fs-read-allowed-paths.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/fs-read-deny-lfa.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/fs-read-deny.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/net-deny-excluded.yamlis excluded by!**/*.yaml
📒 Files selected for processing (62)
Makefilecmd/nuclei/main.gogo.modinternal/tests/integration/runner_test.gointernal/tests/integration/security_hardening_test.gointernal/tests/testutils/integration.gopkg/js/compiler/compiler_test.gopkg/js/libs/dcerpc/dcerpc.gopkg/js/libs/dcerpc/transport_init.gopkg/js/libs/fs/fs.gopkg/js/libs/fs/fs_test.gopkg/js/libs/guard/fastdialer_guard_test.gopkg/js/libs/guard/jslibs_guard_test.gopkg/js/libs/kerberos/sendtokdc.gopkg/js/libs/krbforge/krbforge.gopkg/js/libs/krbforge/krbforge_test.gopkg/js/libs/ldap/ldap.gopkg/js/libs/mssql/mssql.gopkg/js/libs/mysql/mysql.gopkg/js/libs/mysql/mysql_private.gopkg/js/libs/mysql/mysql_private_test.gopkg/js/libs/net/net.gopkg/js/libs/oracle/oracle.gopkg/js/libs/oracle/oracle_test.gopkg/js/libs/oracle/oracledialer.gopkg/js/libs/pop3/pop3.gopkg/js/libs/postgres/postgres.gopkg/js/libs/rdp/rdp.gopkg/js/libs/redis/redis.gopkg/js/libs/rsync/rsync.gopkg/js/libs/smb/smb.gopkg/js/libs/smb/smb_private.gopkg/js/libs/smb/smbghost.gopkg/js/libs/smtp/smtp.gopkg/js/libs/telnet/telnet.gopkg/js/libs/vnc/vnc.gopkg/protocols/code/code.gopkg/protocols/code/sandbox_exec.gopkg/protocols/code/sandbox_exec_linux.gopkg/protocols/code/sandbox_exec_linux_test.gopkg/protocols/code/sandbox_exec_stub.gopkg/protocols/common/protocolinit/init.gopkg/protocols/common/protocolstate/allowlist.gopkg/protocols/common/protocolstate/allowlist_test.gopkg/protocols/common/protocolstate/dial.gopkg/protocols/common/protocolstate/dial_test.gopkg/protocols/common/protocolstate/dsn.gopkg/protocols/common/protocolstate/dsn_test.gopkg/protocols/common/protocolstate/file.gopkg/protocols/common/protocolstate/file_test.gopkg/protocols/common/protocolstate/fsbroker.gopkg/protocols/common/protocolstate/fsbroker_test.gopkg/protocols/common/protocolstate/headless.gopkg/protocols/common/protocolstate/state.gopkg/protocols/common/protocolstate/test_helpers_test.gopkg/protocols/common/sandbox/errors.gopkg/protocols/common/sandbox/sandbox.gopkg/protocols/common/sandbox/sandbox_linux.gopkg/protocols/common/sandbox/sandbox_linux_test.gopkg/protocols/common/sandbox/sandbox_stub.gopkg/protocols/common/sandbox/sandbox_test.gopkg/types/types.go
Mzack9999
marked this pull request as ready for review
July 5, 2026 23:47
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/js/libs/redis/redis.go (1)
155-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dialer fetch/nil-check.
protocolstate.DialAllowedWithExecutionIDalready re-fetches the dialer viaGetDialersWithIdand fails closed whendialer == nil || dialer.Fastdialer == nil, so the pre-check at lines 155–158 duplicates that work (twoGetDialersWithIdcalls) and can be dropped. It's harmless, so this is optional.♻️ Proposed simplification
func isAuthenticated(ctx context.Context, executionId string, host string, port int) (bool, error) { plugin := pluginsredis.REDISPlugin{} timeout := 5 * time.Second - dialer := protocolstate.GetDialersWithId(executionId) - if dialer == nil { - return false, fmt.Errorf("dialers not initialized for %s", executionId) - } conn, err := protocolstate.DialAllowedWithExecutionID(ctx, executionId, "tcp", net.JoinHostPort(host, fmt.Sprintf("%d", port)))🤖 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/js/libs/redis/redis.go` around lines 155 - 160, The dialer pre-check in the Redis connection path is redundant because DialAllowedWithExecutionID already performs the GetDialersWithId lookup and nil/fail-closed handling. Remove the standalone dialer := protocolstate.GetDialersWithId(executionId) and nil check in the Redis dial logic, and rely on protocolstate.DialAllowedWithExecutionID to enforce the initialization guard while keeping the existing connection flow unchanged.
🤖 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/js/libs/redis/redis.go`:
- Around line 155-160: The dialer pre-check in the Redis connection path is
redundant because DialAllowedWithExecutionID already performs the
GetDialersWithId lookup and nil/fail-closed handling. Remove the standalone
dialer := protocolstate.GetDialersWithId(executionId) and nil check in the Redis
dial logic, and rely on protocolstate.DialAllowedWithExecutionID to enforce the
initialization guard while keeping the existing connection flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b2060a5-74fb-4ab7-af68-1fa0a5f682de
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.modinternal/tests/integration/runner_test.gopkg/js/compiler/compiler_test.gopkg/js/libs/fs/fs_test.gopkg/js/libs/guard/jslibs_guard_test.gopkg/js/libs/krbforge/krbforge_test.gopkg/js/libs/ldap/ldap.gopkg/js/libs/oracle/oracle_test.gopkg/js/libs/postgres/postgres.gopkg/js/libs/redis/redis.gopkg/protocols/code/code.gopkg/protocols/code/code_test.gopkg/protocols/code/sandbox_exec.gopkg/protocols/code/sandbox_exec_test.gopkg/protocols/common/protocolstate/allowlist.gopkg/protocols/common/protocolstate/dial_test.gopkg/protocols/common/protocolstate/file.gopkg/protocols/common/protocolstate/file_test.gopkg/protocols/common/protocolstate/fsbroker_test.gopkg/protocols/common/protocolstate/headless.gopkg/protocols/common/protocolstate/state.gopkg/protocols/common/sandbox/sandbox.gopkg/protocols/common/sandbox/sandbox_linux.gopkg/types/types.go
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/tests/integration/runner_test.go
- pkg/js/libs/fs/fs_test.go
- pkg/js/libs/guard/jslibs_guard_test.go
- pkg/types/types.go
- pkg/protocols/common/protocolstate/allowlist.go
- pkg/js/libs/postgres/postgres.go
- pkg/protocols/common/protocolstate/dial_test.go
- pkg/protocols/common/protocolstate/fsbroker_test.go
- pkg/js/libs/krbforge/krbforge_test.go
- pkg/protocols/common/protocolstate/state.go
- pkg/js/libs/oracle/oracle_test.go
- pkg/js/libs/ldap/ldap.go
- pkg/protocols/common/protocolstate/file_test.go
- pkg/protocols/common/protocolstate/headless.go
- pkg/js/compiler/compiler_test.go
# Conflicts: # go.mod # pkg/js/libs/dcerpc/transport_init.go # pkg/js/libs/mssql/mssql.go # pkg/js/libs/mysql/mysql.go # pkg/js/libs/smb/smb.go
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/js/libs/mssql/fingerprint.go (1)
93-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject missing execution IDs instead of panicking.
ctx.Value("executionId").(string)panics for thecontext.Background()fallback, rather than failing closed with an error. Use a shared safe extractor and return an explicit missing-execution-ID error before any policy check or dial.
pkg/js/libs/mssql/fingerprint.go#L93-L96: safely extract the ID before invoking the memoized probe.pkg/js/libs/smb/smb.go#L179-L182: safely extract the ID forListDir.pkg/js/libs/smb/smb.go#L193-L196: safely extract the ID forReadFile.pkg/js/libs/smb/smb.go#L207-L210: safely extract the ID forListTree.pkg/js/libs/smb/smb.go#L221-L227: ensure theConnectSMBInfoModecall path returns the same error.🤖 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/js/libs/mssql/fingerprint.go` around lines 93 - 96, Replace direct executionId type assertions with the shared safe extractor and return its explicit missing-ID error before invoking probes or dialing. Apply this in pkg/js/libs/mssql/fingerprint.go:93-96 and pkg/js/libs/smb/smb.go:179-182, 193-196, and 207-210 for FingerprintMssql, ListDir, ReadFile, and ListTree; ensure pkg/js/libs/smb/smb.go:221-227 propagates the same error through ConnectSMBInfoMode.
🤖 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.
Outside diff comments:
In `@pkg/js/libs/mssql/fingerprint.go`:
- Around line 93-96: Replace direct executionId type assertions with the shared
safe extractor and return its explicit missing-ID error before invoking probes
or dialing. Apply this in pkg/js/libs/mssql/fingerprint.go:93-96 and
pkg/js/libs/smb/smb.go:179-182, 193-196, and 207-210 for FingerprintMssql,
ListDir, ReadFile, and ListTree; ensure pkg/js/libs/smb/smb.go:221-227
propagates the same error through ConnectSMBInfoMode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ffb73b08-38e2-4917-bdb8-5e0d614d8d89
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/nuclei/main.gogo.modpkg/js/libs/dcerpc/dcerpc.gopkg/js/libs/gptransport/dialer.gopkg/js/libs/mssql/fingerprint.gopkg/js/libs/mssql/mssql.gopkg/js/libs/mysql/mysql.gopkg/js/libs/redis/redis.gopkg/js/libs/smb/smb.gopkg/protocols/common/protocolinit/init.go
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- pkg/js/libs/dcerpc/dcerpc.go
- cmd/nuclei/main.go
- pkg/protocols/common/protocolinit/init.go
- pkg/js/libs/redis/redis.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/js/libs/grpc/grpc_test.go`:
- Around line 196-204: The test setup around outside and outsideDir must use a
unique directory created with os.MkdirTemp, rather than a deterministic path
under $HOME. Verify the selected directory is outside the configured filesystem
allowlist before writing healthProtoset(t), accounting for $HOME potentially
residing under the temp root, and retain cleanup for only the newly created
directory.
In `@pkg/js/libs/grpc/invoke.go`:
- Around line 94-100: Update the error handling around ReadFileAllowed in the
protoset-loading flow to distinguish allowlist-denial errors from ordinary
filesystem read failures. Preserve the “protoset path denied” message only for
the specific policy-denial error, and return a separate contextual read-error
message for missing or unreadable files.
🪄 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 Plus
Run ID: 6457261e-7729-4d13-a9fc-40ef3ea495b2
📒 Files selected for processing (2)
pkg/js/libs/grpc/grpc_test.gopkg/js/libs/grpc/invoke.go
Comment on lines
+196
to
204
| // t.TempDir() lives under os.TempDir(), which is always in the filesystem | ||
| // allowlist — place the file under $HOME so it is outside templates/config/temp. | ||
| home, err := os.UserHomeDir() | ||
| require.NoError(t, err) | ||
| outsideDir := filepath.Join(home, ".nuclei-grpc-outside-"+t.Name()) | ||
| require.NoError(t, os.MkdirAll(outsideDir, 0o700)) | ||
| t.Cleanup(func() { _ = os.RemoveAll(outsideDir) }) | ||
| outside := filepath.Join(outsideDir, "health.protoset") | ||
| require.NoError(t, os.WriteFile(outside, healthProtoset(t), 0o600)) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a unique, verified-disallowed directory.
The deterministic $HOME path can already exist, causing MkdirAll to reuse it and cleanup to delete pre-existing user data. Concurrent test runs can also share it. Use os.MkdirTemp and verify the selected parent is outside the configured allowlist; $HOME may itself be under the temp root.
🤖 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/js/libs/grpc/grpc_test.go` around lines 196 - 204, The test setup around
outside and outsideDir must use a unique directory created with os.MkdirTemp,
rather than a deterministic path under $HOME. Verify the selected directory is
outside the configured filesystem allowlist before writing healthProtoset(t),
accounting for $HOME potentially residing under the temp root, and retain
cleanup for only the newly created directory.
Comment on lines
+94
to
100
| // resolve + read through the filesystem allowlist: unless -lfa / | ||
| // --allowed-paths expand it, only templates/config/temp roots are | ||
| // permitted. | ||
| data, err := protocolstate.ReadFileAllowed(&types.Options{ExecutionId: executionID}, protosetFile) | ||
| if err != nil { | ||
| return nil, noop, fmt.Errorf("protoset path denied: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve ordinary protoset read errors.
Every ReadFileAllowed failure is returned as "protoset path denied". Missing or unreadable files in an allowed directory will therefore be misreported as policy violations. Map only the allowlist-denial error to this message and return a separate read-error message for other failures.
🤖 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/js/libs/grpc/invoke.go` around lines 94 - 100, Update the error handling
around ReadFileAllowed in the protoset-loading flow to distinguish
allowlist-denial errors from ordinary filesystem read failures. Preserve the
“protoset path denied” message only for the specific policy-denial error, and
return a separate contextual read-error message for missing or unreadable files.
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/js/libs/http/http.go (1)
270-317: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFixed TLS
ServerNamebreaks SNI on cross-host redirects.
tlsConfig.ServerNameis set once from the initial request's host (Lines 275-277) and the same*tls.Configobject is reused byDialTLSContext(Line 285) for the wholedo()call. Redirects to a different host are explicitly permitted (host re-checked viaIsHostAllowedinCheckRedirect, Lines 312-314), but the TLS handshake to that new host will still present the original host asServerName, which can break SNI-based routing on the target and leaks the original hostname to an unrelated server.🐛 Proposed fix: derive ServerName per-dial from the actual address
- tlsConfig := &tls.Config{ - InsecureSkipVerify: true, - MinVersion: tls.VersionTLS10, - Renegotiation: tls.RenegotiateOnceAsClient, - } - if host != "" { - tlsConfig.ServerName = host - } - transport := &http.Transport{ Proxy: http.ProxyFromEnvironment, DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { return protocolstate.DialAllowedWithExecutionID(ctx, executionID, network, addr) }, DialTLSContext: func(ctx context.Context, network, addr string) (net.Conn, error) { - return protocolstate.DialTLSAllowedWithExecutionID(ctx, executionID, network, addr, tlsConfig) + cfg := &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS10, + Renegotiation: tls.RenegotiateOnceAsClient, + } + if h, _, err := net.SplitHostPort(addr); err == nil && h != "" { + cfg.ServerName = h + } + return protocolstate.DialTLSAllowedWithExecutionID(ctx, executionID, network, addr, cfg) },🤖 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/js/libs/http/http.go` around lines 270 - 317, Update the TLS setup used by the HTTP client and its DialTLSContext callback so ServerName is derived for each connection from the actual destination address, rather than remaining fixed to the initial host across redirects. Preserve the existing host-allowance validation in CheckRedirect and TLS configuration, while ensuring cross-host redirects send the redirected hostname during handshake.
🤖 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.
Outside diff comments:
In `@pkg/js/libs/http/http.go`:
- Around line 270-317: Update the TLS setup used by the HTTP client and its
DialTLSContext callback so ServerName is derived for each connection from the
actual destination address, rather than remaining fixed to the initial host
across redirects. Preserve the existing host-allowance validation in
CheckRedirect and TLS configuration, while ensuring cross-host redirects send
the redirected hostname during handshake.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f2533a4-bf42-4758-aec0-0989e1750371
📒 Files selected for processing (1)
pkg/js/libs/http/http.go
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.
Summary
protocolstate.DialAllowed; dial policy is fail-closed when dialers are missing.-lfafull bypass with an allowlist (templates, temp, config, cwd with-lfa,--allowed-paths).ReadFileAllowed,NormalizePath, MySQL/Oracle/MSSQL/Postgres sanitizers).--no-sandboxto disable); stub elsewhere.bwrapworks; Docker sandbox in template still wins.pkg/js/libs/guardblocking rawnet/os/execand directFastdialer.Dial.security-hardening;make test-sandbox-linuxfor landlock + bwrap on Linux.Test plan
go test ./pkg/protocols/common/protocolstate/... ./pkg/protocols/common/sandbox/... ./pkg/js/libs/guard/... ./pkg/js/libs/fs/... ./pkg/protocols/code/...go test -tags=integration ./internal/tests/integration -run security-hardeningmake test-sandbox-linux(Docker + bubblewrap + landlock)Notes
gozero.Evalif namespaces aren't available (common in Docker without privileges).Summary by CodeRabbit
--allowed-pathsto extend permitted local roots (when local file access is enabled).--no-sandboxto disable OS-level filesystem sandbox enforcement.--allow-local-file-accesshelp to clarify it’s directory-limited.Closes #7526