Read real Windows ACLs in ai_tools fsutil.Stat - #50772
Conversation
Resolves #50703 Stat() returned Perm{Known: false} unconditionally on Windows, so the world_writable risk flag on agent_instruction rows could never fire no matter what the file's real ACL was. Split the permission read per platform: perm_unix.go keeps the mode-bit logic, perm_windows.go reads the DACL via GetSecurityInfo, and perm_acl.go holds the platform-neutral ACE decision rules so they are unit-testable anywhere. Known: false now means "could not determine" rather than "Windows". Everyone (S-1-1-0) and Authenticated Users (S-1-5-11) count as world; BUILTIN\Users is excluded because standard locations grant it create rights by default. Also adds ./orbit/pkg/table/ai_tools/... to the Windows CI package list, which otherwise never exercises this code.
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThe change adds cross-platform permission detection for 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@orbit/pkg/table/ai_tools/internal/fsutil/perm_acl.go`:
- Around line 67-72: Update the ACL evaluation logic around the write decision
handling to track FILE_WRITE_DATA, FILE_APPEND_DATA, DELETE, WRITE_DAC, and
WRITE_OWNER independently rather than collapsing them through writeMask. Set
WorldWritable when any of these effective capabilities is allowed, while
preserving per-capability deny precedence; add a regression case covering DELETE
denied followed by FILE_WRITE_DATA allowed.
In `@orbit/pkg/table/ai_tools/internal/fsutil/perm_unix.go`:
- Around line 11-19: Update the permission lookup around os.Lstat in the
relevant Unix permission function to reject non-regular files, including
symlinks, by returning Perm{Known: false} before evaluating mode bits. Only
compute WorldReadable and WorldWritable for regular files, matching the
OpenRegular behavior on Windows.
In `@orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go`:
- Around line 42-45: In the DACL evaluation logic, make incomplete or
unsupported evaluation return an unknown posture: at the AceCount limit check,
return Perm{} when truncation would occur instead of evaluating a shortened ACL,
and in the non-inherit-only unsupported ACE branch return Perm{} rather than
skipping it. Add tests covering both cases and verify Known is false.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 538a6a5f-9195-44dd-ae22-247c041bda8f
📒 Files selected for processing (7)
.github/workflows/test-go-windows.ymlorbit/pkg/table/ai_tools/internal/fsutil/fsutil.goorbit/pkg/table/ai_tools/internal/fsutil/perm_acl.goorbit/pkg/table/ai_tools/internal/fsutil/perm_acl_test.goorbit/pkg/table/ai_tools/internal/fsutil/perm_unix.goorbit/pkg/table/ai_tools/internal/fsutil/perm_windows.goorbit/pkg/table/ai_tools/internal/fsutil/perm_windows_test.go
| if !readDecided && a.Mask&readMask != 0 { | ||
| read, readDecided = a.Allow, true | ||
| } | ||
| if !writeDecided && a.Mask&writeMask != 0 { | ||
| write, writeDecided = a.Allow, true | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Evaluate each write capability separately.
Lines 70-71 treat any deny that intersects writeMask as a denial of all write access. A DACL that denies DELETE and later allows FILE_WRITE_DATA still permits a world principal to change file content. The current code returns WorldWritable: false.
Track decisions for each effective capability, including FILE_WRITE_DATA, FILE_APPEND_DATA, DELETE, WRITE_DAC, and WRITE_OWNER. Set WorldWritable when any capability is allowed. Add a regression case for a deny of DELETE followed by an allow of FILE_WRITE_DATA.
🤖 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 `@orbit/pkg/table/ai_tools/internal/fsutil/perm_acl.go` around lines 67 - 72,
Update the ACL evaluation logic around the write decision handling to track
FILE_WRITE_DATA, FILE_APPEND_DATA, DELETE, WRITE_DAC, and WRITE_OWNER
independently rather than collapsing them through writeMask. Set WorldWritable
when any of these effective capabilities is allowed, while preserving
per-capability deny precedence; add a regression case covering DELETE denied
followed by FILE_WRITE_DATA allowed.
| fi, err := os.Lstat(path) | ||
| if err != nil { | ||
| return Perm{} | ||
| } | ||
| m := fi.Mode().Perm() | ||
| return Perm{ | ||
| WorldReadable: m&0o044 != 0, | ||
| WorldWritable: m&0o022 != 0, | ||
| Known: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify symlink mode bits as file permissions.
os.Lstat returns the symlink mode. Those mode bits do not control access to the link and are commonly 0777. This marks a symlinked instruction file as world-readable and world-writable even when no user can modify its target.
Reject non-regular files with Known: false, consistent with the Windows OpenRegular 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 `@orbit/pkg/table/ai_tools/internal/fsutil/perm_unix.go` around lines 11 - 19,
Update the permission lookup around os.Lstat in the relevant Unix permission
function to reject non-regular files, including symlinks, by returning
Perm{Known: false} before evaluating mode bits. Only compute WorldReadable and
WorldWritable for regular files, matching the OpenRegular behavior on Windows.
| count := int(dacl.AceCount) | ||
| if count > maxACEs { | ||
| count = maxACEs | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Return an unknown posture after incomplete DACL evaluation.
The implementation returns Known: true after it omits part of a DACL. This can hide an applicable world allow ACE and suppress the risk flag.
orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go#L42-L45: IfAceCountexceedsmaxACEs, returnPerm{}instead of evaluating a truncated ACL.orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go#L56-L57: If a non-inherit-only ACE type is unsupported, returnPerm{}instead of skipping it.
Add tests for both cases. This preserves the documented rule that Known is false when permission evaluation cannot determine the posture.
📍 Affects 1 file
orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go#L42-L45(this comment)orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go#L56-L57
🤖 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 `@orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go` around lines 42 -
45, In the DACL evaluation logic, make incomplete or unsupported evaluation
return an unknown posture: at the AceCount limit check, return Perm{} when
truncation would occur instead of evaluating a shortened ACL, and in the
non-inherit-only unsupported ACE branch return Perm{} rather than skipping it.
Add tests covering both cases and verify Known is false.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50772 +/- ##
==========================================
+ Coverage 68.35% 68.38% +0.03%
==========================================
Files 3957 3963 +6
Lines 254097 254959 +862
Branches 13530 13530
==========================================
+ Hits 173681 174350 +669
- Misses 64860 65015 +155
- Partials 15556 15594 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Resolves #50703
Stat() returned Perm{Known: false} unconditionally on Windows, so the world_writable risk flag on agent_instruction rows could never fire no matter what the file's real ACL was.
Split the permission read per platform: perm_unix.go keeps the mode-bit logic, perm_windows.go reads the DACL via GetSecurityInfo, and perm_acl.go holds the platform-neutral ACE decision rules so they are unit-testable anywhere. Known: false now means "could not determine" rather than "Windows".
Everyone (S-1-1-0) and Authenticated Users (S-1-5-11) count as world; BUILTIN\Users is excluded because standard locations grant it create rights by default.
Also adds ./orbit/pkg/table/ai_tools/... to the Windows CI package list, which otherwise never exercises this code.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Bug Fixes