Add sandboxed file() DSL helper for static payloads - #7583
Conversation
|
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:
WalkthroughAdds a sandboxed ChangesFile helper execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TemplateExecuter
participant WithFileLoadContext
participant file
participant Options.LoadHelperFile
TemplateExecuter->>WithFileLoadContext: Install executor file context
WithFileLoadContext->>file: Evaluate file(path)
file->>Options.LoadHelperFile: Load sandboxed helper file
Options.LoadHelperFile-->>file: Return validated reader
file-->>TemplateExecuter: Return file contents
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/operators/common/dsl/goid.go (1)
9-25: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffGoroutine-ID via
runtime.Stackparsing is an unsupported Go trick.
goid()derives goroutine identity by parsing the text ofruntime.Stack, which is documented for human-readable debug output only, not a stable public contract. If the trace format ever changes,goid()silently falls back to0for every goroutine, collapsing all concurrently-installedWithFileLoadContextsandboxes into one shared slot — a correctness/sandbox-isolation regression that would be hard to notice.Consider using an actively-maintained library that already handles the fragile stack-parsing edge cases (e.g.
petermattis/goid), or better, thread theFileLoadContextexplicitly through the call chain (e.g., viacontext.Context) instead of relying on ambient goroutine-local state.🤖 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/operators/common/dsl/goid.go` around lines 9 - 25, Replace the unsupported runtime.Stack parsing in goid with an actively maintained goroutine-ID mechanism such as petermattis/goid, or refactor the file-load sandbox flow to pass FileLoadContext explicitly through the call chain. Update the surrounding WithFileLoadContext and file() integration so concurrent sandboxes remain isolated and avoid silently sharing a zero-ID slot.
🤖 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/variables/variables.go`:
- Around line 240-245: Update the file() detection in the variable analysis
logic to recognize file() only as a parsed DSL expression or through a
token-aware matcher, rather than using strings.Contains on the raw value.
Preserve lazy evaluation for genuine file() expressions while avoiding matches
inside ordinary text such as "profile(". Add a regression test covering literal
text containing file(.
In `@pkg/tmplexec/exec.go`:
- Around line 335-344: Update TemplateExecuter.fileLoadContext to lazily create
and retain a single *dsl.FileLoadContext per executor, using a synchronization
mechanism such as sync.Once for concurrent calls. Reuse that context across
Compile, Execute, and ExecuteWithResults invocations so its existing sync.Map
cache persists across targets, while preserving the nil behavior for an
uninitialized executer or options.
---
Nitpick comments:
In `@pkg/operators/common/dsl/goid.go`:
- Around line 9-25: Replace the unsupported runtime.Stack parsing in goid with
an actively maintained goroutine-ID mechanism such as petermattis/goid, or
refactor the file-load sandbox flow to pass FileLoadContext explicitly through
the call chain. Update the surrounding WithFileLoadContext and file()
integration so concurrent sandboxes remain isolated and avoid silently sharing a
zero-ID slot.
🪄 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: 9be4db4f-065a-4d20-8be2-915ba1a64192
📒 Files selected for processing (7)
pkg/operators/common/dsl/dsl.gopkg/operators/common/dsl/file.gopkg/operators/common/dsl/file_test.gopkg/operators/common/dsl/goid.gopkg/protocols/common/variables/variables.gopkg/protocols/common/variables/variables_test.gopkg/tmplexec/exec.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operators/common/dsl/file_security_test.go (1)
65-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: fold these two cases into the initial
casesslice literal.Repeating the anonymous
struct{ name string; expr string }type just to append two more entries is unnecessary; they can be added directly to the slice literal at lines 45-63.♻️ Proposed simplification
{name: "dot_path", expr: `file(".")`}, {name: "dotdot_path", expr: `file("..")`}, {name: "empty_components", expr: `file("////")`}, + {name: "template_directory", expr: fileExpr(templateDir)}, + {name: "templates_directory", expr: fileExpr(templatesDir)}, } - - // Also deny reading the template directory itself and templates dir as paths. - cases = append(cases, - struct { - name string - expr string - }{name: "template_directory", expr: fileExpr(templateDir)}, - struct { - name string - expr string - }{name: "templates_directory", expr: fileExpr(templatesDir)}, - )🤖 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/operators/common/dsl/file_security_test.go` around lines 65 - 75, Update the initial cases slice literal in the file-security test to include the template_directory and templates_directory entries directly, and remove the later append block and its repeated anonymous struct declarations.
🤖 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/operators/common/dsl/file_security_test.go`:
- Around line 65-75: Update the initial cases slice literal in the file-security
test to include the template_directory and templates_directory entries directly,
and remove the later append block and its repeated anonymous struct
declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2b94c3f-266d-4fff-95f4-4c4876ddcb84
📒 Files selected for processing (5)
pkg/operators/common/dsl/file_security_test.gopkg/operators/common/dsl/file_test.gopkg/protocols/common/variables/variables.gopkg/protocols/common/variables/variables_test.gopkg/tmplexec/exec.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/protocols/common/variables/variables_test.go
- pkg/protocols/common/variables/variables.go
- pkg/operators/common/dsl/file_test.go
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/operators/common/dsl/file_security_unix_test.go`:
- Around line 60-69: Strengthen the error assertion in the FIFO rejection test
to require the helper’s specific non-regular-file error or sentinel, rather than
accepting generic “could not load”, “denied”, or “device” messages. Retain only
error variants that are genuinely platform-specific while preserving the
existing require.Error check and errCh flow.
- Around line 39-58: Update the FIFO setup around writerCh and the file()
rejection check so observing the evaluator error does not depend on a blocking
O_WRONLY open completing. Use a non-blocking or RDWR FIFO opener, or select
concurrently between writerCh and errCh, while preserving cleanup for any
successfully opened writer and the existing rejection assertion.
🪄 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: 8e7cd202-7fc6-4e00-a0c2-b8581289ac2f
📒 Files selected for processing (2)
pkg/operators/common/dsl/file_security_test.gopkg/operators/common/dsl/file_security_unix_test.go
💤 Files with no reviewable changes (1)
- pkg/operators/common/dsl/file_security_test.go
1 participant
Summary
file(path)DSL helper that returns whole-file contents (including binary) for static payloadsLoadHelperFilesandbox (templates dir, template-relative paths,-lfa, hard-link checks) with size and regular-file guardsCloses #5402
Summary by CodeRabbit
file()helper to safely load file contents during template evaluation, using sandboxed, template-relative resolution and per-execution caching.goid()helper for DSL use.file()detection to trigger lazy evaluation only when an actual helper call is present.