-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add defaultDenyRead mode for strict filesystem isolation #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/sandbox/linux.go">
<violation number="1" location="internal/sandbox/linux.go:370">
P2: `defaultDenyRead` skips mounting `/sys`, even though it is listed as an essential readable path. In strict mode this makes `/sys` inaccessible and can break tools that rely on sysfs. Remove `/sys` from the skip list so it is bound read-only with the other default paths.</violation>
</file>
<file name="internal/sandbox/dangerous.go">
<violation number="1" location="internal/sandbox/dangerous.go:88">
P2: Allowlisting the entire `/var` directory is overly broad for strict read isolation and can expose system logs/caches. Narrow this to the specific subpaths required (e.g., `/var/run`) and require explicit allowRead for anything else.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| { | ||
| "extends": "code", | ||
| "filesystem": { | ||
| // Deny reads by default, only system paths and allowRead are accessible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jy-tan ,
Are you sure these comments are OK? Normally comments cannot be used in JSON (https://stackoverflow.com/questions/244777/can-comments-be-used-in-json).
If the template is automatically stripped out of these, then all good. If not, it's always possible that this goes in corrupted and unnoticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we use https://github.com/tidwall/jsonc to parse files in JSONC. See:
fence/internal/templates/templates.go
Line 107 in b14f707
| if err := json.Unmarshal(jsonc.ToJSON(data), &cfg); err != nil { |
Summary
Introduces a new
defaultDenyReadfilesystem restriction mode that inverts the default read policy. Instead of allowing all reads and selectively denying paths, this mode denies all reads by default and only permits access to essential system paths and explicitly allowed directories.This is useful for AI coding agents that should only be able to read the project they're working on, not browse arbitrary files on the filesystem.
Resolves #18.
Changes
defaultDenyReadboolean toFilesystemConfigfor strict read isolationallowReadarray to whitelist specific paths whendefaultDenyReadis enabledGetDefaultReadablePaths()returning essential system paths (binaries, libs, dev tools, etc.)file-read-metadatafor directory traversal (stat, readdir)file-read-dataonly for explicitly allowed pathscode-stricttemplate that extendscodewithdefaultDenyReadenabledallowReadpathsallowReadarrays and ORdefaultDenyReadbooleans